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.

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

Avoid sorting CallableCustomMethodPointers by their actual address values #72346

Merged
merged 1 commit into from
Jul 26, 2023
Merged

Avoid sorting CallableCustomMethodPointers by their actual address values #72346

merged 1 commit into from
Jul 26, 2023

Conversation

myaaaaaaaaa
Copy link
Contributor

#69874 is caused by signals being stored inside a VMap<Callable, ...>. This means that Callables that are allocated later would have a larger address and end up being sorted to the end of the VMap, making them easier to push/pop... until the C runtime starts recycling addresses, and connecting/disconnecting lots of signals becomes inexplicably slow.

This patch "fixes" that problem in that it makes connecting/disconnecting consistently slow, so that signal performance problems become easier to diagnose.

@myaaaaaaaaa myaaaaaaaaa requested review from a team as code owners January 30, 2023 02:13
@myaaaaaaaaa
Copy link
Contributor Author

Switched to a cleaner fix that just uses memcmp, to remove the need for the ifdef.

Because of how byte ordering works on little-endian machines, this will cause pointer digits to be compared from right to left, so they will no longer be sorted in a meaningful order.

@myaaaaaaaaa myaaaaaaaaa changed the title Sort CallableCustomMethodPointers by hash to avoid performance dependency on allocation order Avoid sorting CallableCustomMethodPointers by their actual address values Feb 9, 2023
@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 19, 2023
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.

My "backwords" wasn't a typo, but, fun enough, "backwards" means the same in this context. 😃

@myaaaaaaaaa
Copy link
Contributor Author

myaaaaaaaaa commented Jun 20, 2023

My "backwords" wasn't a typo, but, fun enough, "backwards" means the same in this context. 😃

It failed CI, so I had no choice 🙃

@YuriSizov YuriSizov merged commit 53ba9cc into godotengine:master Jul 26, 2023
13 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants