Prefer inline representation over static#278
Merged
Merged
Conversation
6dc4cd1 to
0ceca1f
Compare
Contributor
Author
|
Would anyone have time to review this please? I believe this represents a significant performance boost for some common use cases, with no apparent downside. |
SimonSapin
reviewed
Jul 17, 2024
f534a9b to
9db0317
Compare
Contributor
Author
|
Have rebased on top of #277 which is now based on latest main. Also made changes to improve code style (as mentioned above, I was very new to Rust when I made this PR last year so, while I believe code was correct, it wasn't very idiomatic). |
b84bfc0 to
c6ac6ab
Compare
c6ac6ab to
538f87d
Compare
jdm
approved these changes
Jul 31, 2024
Member
jdm
left a comment
There was a problem hiding this comment.
This looks reasonable! Apologies for the long delay, and thanks for the clear motivation for the change!
Contributor
Author
|
No worries. Really glad to see this merged! |
This file contains hidden or 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
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.
As mentioned in #276, current behavior is that short strings can be stored as static strings or inline. This adds an overhead for interning short strings, as they need to be hashed and compared to possible matches in the static set.
This PR changes the behavior to always store strings of 7 bytes or less inline.
This greatly improves the performance of interning short strings - approx 250% speedup, depending on length of the string.
The benchmarks in this repo show no apparent negative effect on any other operations (though there's a lot of noise in the benchmarks).
This PR does not involve any breaking changes.
Even though interning short static atoms is very fast anyway, I imagine this change will translate to a significant real-world performance improvement for some use cases. For example, SWC uses string-cache when parsing Javascript which typically includes a lot of short variable names and tokens, especially in minified code e.g.
for (let i = 0; i < len; i++).Notes on implementation
StaticAtomSet::empty_string_indexwould need to be removed otherwise).pack_inline()calls in place ofpack_static()to generate inline atoms at compile time.