-
-
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
[Merged by Bors] - Remove string-interner
dependency and implement custom string Interner
#2147
Conversation
a33e8aa
to
28f9272
Compare
28f9272
to
4958d0f
Compare
Test262 conformance changesVM implementation
|
Codecov Report
@@ Coverage Diff @@
## main #2147 +/- ##
==========================================
+ Coverage 42.68% 42.71% +0.02%
==========================================
Files 222 225 +3
Lines 20763 20791 +28
==========================================
+ Hits 8863 8881 +18
- Misses 11900 11910 +10
Continue to review full report at Codecov.
|
Benchmark for cb3dc61Click to view benchmark
|
Benchmark for 234d628Click to view benchmark
|
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.
This looks good :) what are the main differences with the upstream implementation?
I just noticed that some documentation links are failing (give a look to the lints in GitHub, some are even in non-modified parts of the code).
Essentially I've inlined the However, I'm expecting the most important improvement will come with the implementation of |
Benchmark for ef932a1Click to view benchmark
|
Benchmark for b1f06a6Click to view benchmark
|
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.
Very nice comments explaining how this works and also really good abstractions to make the behavior clear. Really good work :)
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.
Looks great :) I noticed some things that could be potentially improved. Feel free to take them into account, and we can merge this :)
ccea232
to
7fb8d9d
Compare
Benchmark for a24b823Click to view benchmark
|
Benchmark for 75c2a77Click to view benchmark
|
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.
Looking good, still just a comment about safety in edge case scenarios.
7fb8d9d
to
0c2ef02
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.
Looks great now :) it seems that boa_engine/src/builtins/function/mod.rs
is giving a compile error due to Sym
not implementing Trace
. I guess the solution should be similar to Object
.
@Razican fixed! |
Benchmark for 86598bdClick to view benchmark
|
Benchmark for d1774c3Click to view benchmark
|
bors r+ |
…rner` (#2147) So, @raskad and myself had a short discussion about the state of #736, and we came to the conclusion that it would be a good time to implement our own string interner; partly because the `string-interner` crate is a bit unmaintained (as shown by Robbepop/string-interner#42 and Robbepop/string-interner#47), and partly because it would be hard to experiment with custom optimizations for UTF-16 strings. I still want to thank @Robbepop for the original implementation though, because some parts of this design have been shamelessly stolen from it 😅. Having said that, this PR is a complete reimplementation of the interner, but with some modifications to (hopefully!) make it a bit easier to experiment with UTF-16 strings, apply optimizations, and whatnot :)
Pull request successfully merged into main. Build succeeded: |
string-interner
dependency and implement custom string Interner
string-interner
dependency and implement custom string Interner
So, @raskad and myself had a short discussion about the state of #736, and we came to the conclusion that it would be a good time to implement our own string interner; partly because the
string-interner
crate is a bit unmaintained (as shown by Robbepop/string-interner#42 and Robbepop/string-interner#47), and partly because it would be hard to experiment with custom optimizations for UTF-16 strings. I still want to thank @Robbepop for the original implementation though, because some parts of this design have been shamelessly stolen from it 😅.Having said that, this PR is a complete reimplementation of the interner, but with some modifications to (hopefully!) make it a bit easier to experiment with UTF-16 strings, apply optimizations, and whatnot :)