-
-
Notifications
You must be signed in to change notification settings - Fork 411
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
Remove direct conversion from &str
to JsValue
/PropertyKey
.
#3319
Conversation
Test262 conformance changes
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3319 +/- ##
==========================================
+ Coverage 49.69% 49.72% +0.03%
==========================================
Files 443 443
Lines 43167 43378 +211
==========================================
+ Hits 21451 21569 +118
- Misses 21716 21809 +93
☔ View full report in Codecov by Sentry. |
Benchmark for 69d2900Click to view benchmark
|
Ok, results are a bit erratic (+293.51% in parsing string copy without even touching |
Yep, some nice wins by skipping the conversions. This PR uses 573KB: Mainly because conversions use an additional 24KB of intermediary allocations: It would be cool if we could also remove the additional 12KB used by the thread local cache of static strings (maybe a phf?): |
dc7f7af
to
c98c4da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the change. The cost of conversions like this should be very clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this! I am a bit curious about documentation on using js_string
over &str
for contributors and anybody consuming the API. But I'm fine with merging as is.
This is with the objective of avoiding hidden conversions while doing the conversion from a string literal to a
JsValue
. Removing the direct conversion toJsValue
makes it a bit easier to realize that this is doing a costly conversion ofstring literal -> (runtime) decode UTF-8 to UTF-16 -> (runtime) JsString -> (runtime) JsValue
and pushes the alternative of using the
js_string
macro that makes the optimal conversionstring literal -> (compile time) &[u16] -> (runtime) JsString -> (runtime) JsValue
This should technically improve realm initialization times, but let's see what the benchmarks show.