Change the internal structure of JsString for performance#4455
Change the internal structure of JsString for performance#4455hansl merged 7 commits intoboa-dev:mainfrom
Conversation
Test262 conformance changes
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4455 +/- ##
==========================================
+ Coverage 47.24% 57.19% +9.95%
==========================================
Files 476 508 +32
Lines 46892 58027 +11135
==========================================
+ Hits 22154 33190 +11036
- Misses 24738 24837 +99 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This is the first in a series of PR to improve the performance of strings in Boa. The first step was to introduce a new type of strings, `SliceString`, which contains a strong pointer to another string and start/end indices. This allows for very fast slicing of strings. This initially came at a performance cost by having an enumeration of kinds of strings. An intermediate experiment was introduced to have the kind be a tag on the internal JsString pointer. This still came as a cost as it required bit operations to figure out which function to call. Finally, I moved to using a `vtable`. This helped with many points: 1. as fast as before. Before this PR, there was still a deref of a pointer when accessing internal fields. 2. we can now introduce many other types (which will come in their separate PRs). 3. this makes the code to clone/drop/as_str (and even construction) more streamline as each function is their own implementation.
d7de465 to
0093332
Compare
nekevss
left a comment
There was a problem hiding this comment.
Reviewed! Had a couple comments and nits. Overall this is looking good.
| StaticString::new(JsStr::latin1("until".as_bytes())), | ||
| StaticString::new(JsStr::latin1("since".as_bytes())), | ||
| StaticString::new(JsStr::latin1("equals".as_bytes())), | ||
| StaticString::new(JsStr::latin1("toZonedDateTime".as_bytes())), |
There was a problem hiding this comment.
thought: temporal will probably always be temporal feature flagged because of the dependency, so should we also be feature flagging the StaticStrings that we register.
There was a problem hiding this comment.
I wouldn't worry for now. Maybe we should move these constants to boa_engine where it's the only place they're used, then we should feature flag some. I don't think boa_string should have temporal feature flags, personally.
| #[must_use] | ||
| pub fn into_raw(self) -> NonNull<RawJsString> { | ||
| ManuallyDrop::new(self).ptr | ||
| pub fn into_raw(self) -> NonNull<()> { |
There was a problem hiding this comment.
thought: Is NonNull<()> the best type here.
I'm not sure having APIs that expose and accept a NonNull<()> is the best approach vs. casting to a specific new type (e.g. `NonNull``). This is not something I'm necessarily pushing for being changed. It mostly gave me a bit of a pause when reviewing this.
There was a problem hiding this comment.
Changed it to an opaque type.
This is the first in a series of PR to improve the performance of strings in Boa.
The first step was to introduce a new type of strings,
SliceString, which contains a strong pointer to another string and start/end indices. This allows for very fast slicing of strings. This initially came at a performance cost by having an enumeration of kinds of strings. An intermediate experiment was introduced to have the kind be a tag on the internal JsString pointer. This still came as a cost as it required bit operations to figure out which function to call.Finally, I moved to using a
vtable. This helped with many points:The biggest drawback is now we are operating with a bit more pointer and unsafe code. I made sure that all code was passing both debug, release and miri tests on boa_string and boa_engine's builtins::string suite.
This
vtablebring slightly better performance (depending on the test, about 90-110%, but overall 2-3% better than themainbranch), even though we only directly improved string slicing operations.The next suite of PRs are going to cover more string types:
Also, there might still be some slicing that creates new strings from existing ones without using SliceString. Those can be improved greatly.
Please note the
benches/scripts/strings/slice.jstest is twice as fast with this branch than withmain.The performance table for this PR: