Add reflect path parsing benchmark #9364
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nicopap
added
C-Performance
tbillington
approved these changes
Aug 25, 2023
Comment on lines
+21
to
+22
| } | ||
| fn random_ident(rng: &mut ChaCha8Rng, f: &mut dyn Write) { |
| const SIZES: [usize; 6] = [100, 3160, 1000, 3_162, 10_000, 24_000]; | ||
|
|
||
| fn deterministic_rand() -> ChaCha8Rng { | ||
| ChaCha8Rng::seed_from_u64(42) |
There was a problem hiding this comment.
nitpick: would also accept inlining this since it's only used in one place, optionally moving the seed to a named const, but not necessary.
There was a problem hiding this comment.
I re-used the function from benches/benches/bevy_ecs/world/world_get.rs
bevy/benches/benches/bevy_ecs/world/world_get.rs
Lines 28 to 30 in b88ff15
alice-i-cecile
approved these changes
Aug 25, 2023
alice-i-cecile
added
the
S-Ready-For-Final-Review
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 25, 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 (#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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
We want to measure performance on path reflection parsing.
Solution
Benchmark path-based reflection:
ParsedPath::parseIt's fairly noisy, this is why I added the 3% threshold.
Ideally we would fix the noisiness though. Supposedly I'm seeding the RNG correctly, so there shouldn't be much observable variance. Maybe someone can help spot the issue.