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

Make get_short_name return Cow instead of String #9504

Closed
wants to merge 2 commits into from

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Aug 20, 2023

Objective

  • Do not make allocation in get_short_name when not necessary. The get_short_name of a path without generics is strictly a subslice of the path itself, so we don't need to create a new buffer for it.
  • Improve drastically parsing performance by working on ASCII (this allows skipping UTF8 checks (linear with string size) on every slice splits) It is still fully UTF8-complient, for the same reasons as Make the reflect path parser utf-8-unaware #9371
  • Fixes get_short_name should return a Cow most likely #9496

Solution

  • Rework get_short_name to work on &[u8] instead of &str, only returns an owned String when the path contains a special character.
  • Added DisplayShortName. It stores any kind of string, but when displayed, it displays the get_short_name version.

Future work

I think we could realistically replace the GenericTypePathCell with a streaming version of get_short_name that implements Hash, Eq etc. similarly to DisplayShortName. It would add the parsing cost on top of hashing/equality check, but now that the parser is a lot faster, it's questionable whether the cost is actually significant.

We could also replace the CowStr in the scheduling error type with DisplayShortName, to lazily parse the type path only when displaying it.


Changelog

  • get_short_name returns a Cow<str> instead of String, avoiding allocation when not strictly necessary. It is also significantly more performant
  • Added DisplayShortName. It stores any kind of string, but when displayed, it displays the get_short_name version.

Migration Guide

  • get_short_name returns a Cow<str> instead of String, add .to_string() to the return value to get a String. However, we recommend that if you can, you store the Cow over the String to avoid allocations.

@nicopap nicopap added C-Performance A change motivated by improving speed, memory usage or compile times M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide A-Utils Utility functions and types labels Aug 20, 2023
@nicopap nicopap added the C-Feature A new feature, making something new possible label Aug 20, 2023
@nicopap
Copy link
Contributor Author

nicopap commented Aug 20, 2023

On the performance-side of things. I'll assume it's more perfs because:

  1. It's the same optimization as Make the reflect path parser utf-8-unaware #9371, and it has been proven to be a 20% speedup on benchmarks
  2. In the simple case, we completely skip allocation
  3. We pre-allocate to a size known to be able to hold the return value. For a 33 chars string, that saves us 2 reallocations, for 65 chars: 3 etc.
  4. We might elide bound checks (though they were probably mitigated by the optimizer already) by storing the &[u8] instead of storing an offset and getting the sub-slice.

@nicopap nicopap added this to the 0.12 milestone Aug 20, 2023
@alice-i-cecile alice-i-cecile removed the C-Feature A new feature, making something new possible label Aug 20, 2023
@alice-i-cecile
Copy link
Member

I like this change, and agree that it should improve performance without seriously hurting complexity. Can you add a test for a non-ASCII type name to verify that it works correctly (and continues to in the future)?

@hymm
Copy link
Contributor

hymm commented Sep 7, 2023

Could you rebase or merge main? There's some changes I made to schedule.rs that are conflicting. sorry 😅

@nicopap
Copy link
Contributor Author

nicopap commented Sep 7, 2023

There is also the uppercase fix (#9587), which I'm not exactly sure how to integrate in this change.

@nicopap nicopap marked this pull request as draft September 14, 2023 16:27
@alice-i-cecile
Copy link
Member

@nicopap want me to toss Adopt-Me on this?

@alice-i-cecile alice-i-cecile removed this from the 0.12 milestone Sep 30, 2023
@nicopap
Copy link
Contributor Author

nicopap commented Oct 1, 2023

@nicopap want me to toss Adopt-Me on this?

Sure. Should be easy to pick up.

@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Oct 1, 2023
@james7132
Copy link
Member

Closing in favor of #12275.

@james7132 james7132 closed this Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Utils Utility functions and types C-Performance A change motivated by improving speed, memory usage or compile times M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

get_short_name should return a Cow most likely
4 participants