Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Redo of Message Queue #75940

Merged
merged 1 commit into from
Apr 12, 2023
Merged

Redo of Message Queue #75940

merged 1 commit into from
Apr 12, 2023

Conversation

reduz
Copy link
Member

@reduz reduz commented Apr 11, 2023 •
edited by akien-mga
Loading

  • Functionality moved to a base class CallQueue, which will be used for inter-thread communication within the scene.
  • MessageQueue now uses growing pages, starts from a single 4k page.
  • Limit still exists, but because its not allocated by default, it can be much higher.

@reduz reduz requested a review from a team as a code owner April 11, 2023 15:57
@Calinou Calinou added this to the 4.x milestone Apr 11, 2023
@akien-mga akien-mga added the bug label Apr 11, 2023
@akien-mga akien-mga modified the milestones: 4.x, 4.1 Apr 11, 2023
@YuriSizov
Copy link
Contributor

You're missing an empty line before the closing header guard :) This prevents the rest of the builds from running.

@reduz reduz force-pushed the redone-message-queue branch 4 times, most recently from ad79aaf to 757a7ba Compare April 11, 2023 18:50
@lawnjelly lawnjelly mentioned this pull request Apr 11, 2023
Closed
@akien-mga akien-mga mentioned this pull request Apr 11, 2023
Closed
@reduz reduz force-pushed the redone-message-queue branch from 757a7ba to 8bc2228 Compare April 11, 2023 20:12
@reduz reduz requested a review from a team as a code owner April 11, 2023 20:12
@reduz reduz force-pushed the redone-message-queue branch from 8bc2228 to 527131c Compare April 11, 2023 21:47
akien-mga
akien-mga reviewed Apr 12, 2023
View reviewed changes
core/object/message_queue.h Show resolved Hide resolved
core/object/message_queue.h Outdated Show resolved Hide resolved
core/object/message_queue.cpp Outdated Show resolved Hide resolved
@reduz reduz force-pushed the redone-message-queue branch 2 times, most recently from 66afb49 to f614101 Compare April 12, 2023 09:16
RandomShaper
RandomShaper reviewed Apr 12, 2023
View reviewed changes
Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, besides a few comments I've added.

core/object/message_queue.cpp Outdated Show resolved Hide resolved
core/object/message_queue.h Show resolved Hide resolved
core/object/message_queue.h Show resolved Hide resolved
core/object/message_queue.cpp Outdated Show resolved Hide resolved
core/object/message_queue.cpp Show resolved Hide resolved
core/object/message_queue.h Show resolved Hide resolved
lawnjelly
lawnjelly reviewed Apr 12, 2023
View reviewed changes
core/object/message_queue.cpp

int room_needed = sizeof(Message) + sizeof(Variant) * p_argcount;
_ensure_first_page();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be done at construction? (Maybe there's a reason for the non-message queues not to have the fixed penalty of 4Kb)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if I understand, but I just used 4k because its the most common value for operating system memory pages, so it can avoid fragmentation. I could have used 8k, but in practice it should not matter because no message should be that big I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking there's no need for the "if check" here if it is done as a one off at construction.

@reduz reduz force-pushed the redone-message-queue branch 2 times, most recently from 5e7079d to 8fd0523 Compare April 12, 2023 12:20
@lawnjelly
Copy link
Member

Also looks fine to me.

RandomShaper
RandomShaper approved these changes Apr 12, 2023
View reviewed changes
Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added new comments about minor things. Good to go in any case.

@reduz
Redo of Message Queue
6055e44 
* Functionality moved to a base class CallQueue, which will be used for inter-thread communication within the scene.
* MessageQueue now uses growing pages, starts from a single 4k page.
* Limit still exists, but because its not allocated by default, it can be much higher.
@reduz reduz force-pushed the redone-message-queue branch from 8fd0523 to 6055e44 Compare April 12, 2023 14:31
@akien-mga akien-mga merged commit 11798fa into godotengine:master Apr 12, 2023
@akien-mga
Copy link
Member

Thanks!

@lawnjelly lawnjelly mentioned this pull request May 5, 2023
Merged
@akien-mga akien-mga mentioned this pull request May 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga left review comments

@RandomShaper RandomShaper RandomShaper approved these changes

@lawnjelly lawnjelly lawnjelly left review comments

Assignees
No one assigned
Labels
bug enhancement topic:core
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.

Change MessageQueue to a page allocator to prevent overflow
6 participants
@reduz @YuriSizov @lawnjelly @akien-mga @RandomShaper @Calinou