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

Optimise ColourInfo conversion and add conversion helper method #5472

Merged
merged 7 commits into from
Oct 24, 2022

Conversation

smoogipoo
Copy link
Contributor

The performance gains here are mostly theoretical and I can't benchmark them meaningfully in a real-world scenario. Consider this more of a refactoring that adds an optimal (and more user-friendly) method for checking whether a ColourInfo represents a single colour and then returning that colour.

This comes off the back of the osu!-side smoke implementation (where I initially noticed this), which potentially uses this method for tens of thousands of vertices (up to 30k is what I was able to achieve after the recent changes). Similar is also done with slider paths (e.g. at some point https://osu.ppy.sh/beatmapsets/756794#osu/1605148 draws some 20k vertices). So at most you'd see ~0.2ms frametime improvement.

Before

Method Mean Error StdDev Allocated
ConvertToSRGBColour 14.31 ns 0.139 ns 0.123 ns -
ConvertToColor4 15.69 ns 0.122 ns 0.102 ns -

After:

Method Mean Error StdDev Allocated
ConvertToSRGBColour 6.9628 ns 0.0684 ns 0.0534 ns -
ConvertToColor4 11.1604 ns 0.1658 ns 0.1551 ns -
ExtractAndConvertToColor4 0.4039 ns 0.0247 ns 0.0231 ns -

It uses concepts I've learnt over the years:

  • Methods with throw are not inlined - extract these methods to promote inlining.
  • Having strings in methods, even if the code is not hit, leads to a significant increase in ASM putting pressure on the JIT.

For comparisons sake, this is the best "real-world" example I could make, by looping TestSceneSmoke over 30 seconds:

before after
a b

Again, I don't think these are indicative of anything so don't put much weight to them. There's too much noise.

@peppy
Copy link
Sponsor Member

peppy commented Oct 21, 2022

Tests not happy 😢

return colour.singleColour;

[DoesNotReturn]
static void throwConversionFromMultiColourToSingleColourException() => throw new InvalidOperationException("Attempted to read single colour from multi-colour ColourInfo.");
Copy link
Contributor

Choose a reason for hiding this comment

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

There already exists a throw helper for InvalidOperationException here.
Maybe we can use this one?

Copy link
Member

Choose a reason for hiding this comment

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

While this is supposed to be a correct use case of that method, because it causes a string construction in the conversion method, which is what's being avoided in this PR.

Copy link
Contributor Author

@smoogipoo smoogipoo Oct 23, 2022

Choose a reason for hiding this comment

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

Technically it's not a string construction, but rather the code constructing the string results in a lot of dead (i.e. never hit) ASM code for the JIT to compile that could also potentially cause the function to not be inlined.

@smoogipoo
Copy link
Contributor Author

Fixed tests, the assertion was added post-fact.

@peppy peppy enabled auto-merge October 24, 2022 03:07
@peppy peppy merged commit 315d1b5 into ppy:master Oct 24, 2022
@smoogipoo smoogipoo deleted the optimise-colourinfo-conversion branch September 11, 2023 02: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