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

Make MessageQueue growable #62303

Closed
wants to merge 1 commit into from
Closed

Make MessageQueue growable #62303

wants to merge 1 commit into from

Conversation

reduz
Copy link
Member

@reduz reduz commented Jun 22, 2022

  • Uses simple vector resizing (po2)

Fixes #35653
Supersedes #35658

@reduz reduz requested a review from a team as a code owner June 22, 2022 08:47
@Calinou Calinou added this to the 4.0 milestone Jun 22, 2022
@akien-mga akien-mga mentioned this pull request Jun 22, 2022
Closed
@lawnjelly
Copy link
Member

lawnjelly commented Jun 22, 2022 •
edited
Loading

The sanitiser is failing in MessageQueue::flush()...

Could this be due to a resize occurring while there is a still a Message * held? Maybe the message is causing further messages to be posted. I don't know if LocalVector guarantees the address to be the same after a resize.

@reduz
Copy link
Member Author

reduz commented Jun 23, 2022

@lawnjelly The address can change, but it should be fine because there are no pointers to anything allocated in there. IMO the sanitizer is butting into something it should not.

@reduz
Make MessageQueue growable
c911001 
* Uses simple vector resizing (po2)

Fixes godotengine#35653
Supersedes godotengine#35658
@reduz reduz force-pushed the growable-message-queue branch from 5396633 to c911001 Compare June 23, 2022 13:26
@reduz reduz requested a review from a team as a code owner June 23, 2022 13:26
akien-mga
akien-mga reviewed Jun 23, 2022
View reviewed changes
doc/classes/ProjectSettings.xml
@@ -1367,8 +1367,7 @@
<member name="layer_names/3d_render/layer_9" type="String" setter="" getter="" default="&quot;&quot;">
Optional name for the 3D render layer 9. If left empty, the layer will display as "Layer 9".
</member>
<member name="memory/limits/message_queue/max_size_kb" type="int" setter="" getter="" default="4096">
Godot uses a message queue to defer some function calls. If you run out of space on it (you will see an error), you can increase the size here.
<member name="memory/limits/message_queue/max_size_mb" type="int" setter="" getter="" default="32">
Copy link
Member

Choose a reason for hiding this comment

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

You need to re-add the description.

@akien-mga akien-mga mentioned this pull request Jun 23, 2022
Merged
@Calinou
Copy link
Member

Calinou commented Jul 16, 2022 •
edited
Loading

Is this backportable for 3.6 (or a 3.5 point release) somehow? I believe this would resolve issues such as #63068.

@lawnjelly
Copy link
Member

lawnjelly commented Jul 16, 2022 •
edited
Loading

Is this backportable for 3.6 (or a 3.5 point release) somehow? I believe this would resolve issues such as #63068.

Should be. 👍

I just did a test for my idea above for the MessageQueue::flush() by modifying the PR slightly to make a copy of the Message rather than use a pointer, and it seems to have passed the CI with the unit tests (well it has passed "Linux Builds / Editor with clang sanitizers" which fails for the original PR), which would seem to confirm the hypothesis, so the sanitizer result may not be a false flag:
https://github.com/lawnjelly/godot/tree/reduz_messagequeue

I was only drawn to this possibility because I've caused similar bugs with vector resizing probably a million times or more. 😁

EDIT: BTW, this patch is primarily to show what might be causing the problem. Making a copy of the Message each time is simple, but depending on how performance sensitive this is, there might be several other ways to solve it.

For instance keeping 2 queues, setting one as read and one as write, that way neither will be treading on the others toes at the same time. Then swapping the 2 queues at the end of MessageQueue::flush() and looping again till both queues are empty. This might use a bit more memory, but avoids copying. Or alternatively using PagedAllocator or similar in some way like the original PR, as the addresses don't move.

@lawnjelly lawnjelly mentioned this pull request Sep 13, 2022
Closed
@akien-mga akien-mga modified the milestones: 4.0, 4.x Jan 25, 2023
@akien-mga akien-mga mentioned this pull request Apr 11, 2023
Merged
@akien-mga
Copy link
Member

Superseded by #75940.

@akien-mga akien-mga closed this Apr 11, 2023
@YuriSizov YuriSizov removed this from the 4.x milestone Dec 2, 2023
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

Assignees
No one assigned
Labels
archived bug needs work topic:core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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