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 the reflect path parser utf-8-unaware #9371

Merged
merged 5 commits into from
Aug 25, 2023

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Aug 6, 2023

Objective

All delimiter symbols used by the path parser are ASCII, this means we can entirely ignore UTF8 handling. This may improve performance.

Solution

Instead of storing the path as an &str + the parser offset, and reading the path using &self.path[self.offset..], we store the parser state in a &[u8]. This allows two optimizations:

  1. Avoid UTF8 checking on &self.path[self.offset..]
  2. Avoid any kind of bound checking, since the length of what is left to read is stored in the &[u8]'s reference metadata, and is assumed valid by the compiler.

This is a major improvement when comparing to the previous parser.

  1. access_following and next_token now inline in PathParser::next
  2. Benchmarking show a 20% performance increase (Add reflect path parsing benchmark #9364)

Please note that while we ignore UTF-8 handling, utf-8 is still supported. This is because we only handle "at the edges" what happens exactly before and after a recognized SYMBOL. utf-8 is handled transparently beyond that.

@nicopap nicopap added C-Performance A change motivated by improving speed, memory usage or compile times A-Reflection Runtime information about types labels Aug 6, 2023
@nicopap nicopap added this to the 0.12 milestone Aug 6, 2023
@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

Copy link
Contributor

@soqb soqb left a comment

Choose a reason for hiding this comment

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

not a huge fan of the unsafe, but it's reasonably trivial and i trust the performance improvements are compelling!

crates/bevy_reflect/src/path/parse.rs Outdated Show resolved Hide resolved
@nicopap nicopap added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 25, 2023
This works because the delimiter symbols '.[]#' are all ASCII, and the
parser really only needs to care about delimiters, so we can avoid the
overhead of handling properly at all steps utf-8 strings.

This is a major improvement when comparing to the previous parser.

1. `access_following` and `next_token` now inline in PathParser::next
2. Benchmarking show a 20% performance increase
auto-merge was automatically disabled August 25, 2023 15:50

Head branch was pushed to by a user without write access

@nicopap
Copy link
Contributor Author

nicopap commented Aug 25, 2023

ooops. Well I rebased to latest HEAD, expecting to fix CI. But instead I disabled auto-merge.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 25, 2023
Merged via the queue into bevyengine:main with commit 365cf31 Aug 25, 2023
20 checks passed
@nicopap nicopap deleted the ascii-path-parser branch August 30, 2023 13:35
@cart cart mentioned this pull request Oct 13, 2023
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants