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

Further reduce allocation overhead in ScrollingHitObjectContainer #26863

Merged
merged 6 commits into from Feb 6, 2024

Conversation

EVAST9919
Copy link
Contributor

@EVAST9919 EVAST9919 commented Jan 31, 2024

Part 2 of #26788

Not sure how safe is making dictionary public though. Would've made it protected, however some mods (such as Depth) will benefit from using that instead of AliveObjects.

master pr
master pr

smoogipoo
smoogipoo previously approved these changes Jan 31, 2024
Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

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

I've changed it to use a ReadOnlyDictionary<> since I don't feel too good about exposing as a Dictionary<>. It may also reduce some allocations due to linq queries in the other places I've adjusted.

@smoogipoo smoogipoo requested a review from bdach January 31, 2024 13:54
@EVAST9919
Copy link
Contributor Author

I've changed it to use a ReadOnlyDictionary

With that half of the allocation came back (1st item)
Снимок экрана 2024-01-31 213019
But I guess there's nothing we can do with that for now?

@smoogipoo
Copy link
Contributor

[rant]
What... I looked into the source and it looked like it exposed the enumerator as the raw struct, but now I check again and it's actually boxing it. I can't believe this is still a thing even in .NET 8.
[/rant]

I've proposed ppy/osu-framework#6163 framework-side that could be used here.

@smoogipoo smoogipoo merged commit ca36919 into ppy:master Feb 6, 2024
15 of 17 checks passed
@EVAST9919 EVAST9919 deleted the scrolling-alloc branch February 6, 2024 17:25
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.

None yet

4 participants