[Merged by Bors] - Interner support in the parser #1765
Closed
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
Razican
added
enhancement
|
I went through most files (skipped test files that looked like simple refactorings). Looks good to me so far. |
Razican
force-pushed
the
feature/parser_interner
branch
2 times, most recently
from
786ec51
to
f9e81a9
Compare
|
I got this to compile, but it seems I made a mistake somewhere, because it's failing the tests. |
Razican
requested review from
raskad,
HalidOdat,
0x7D2B,
jasonwilliams,
jedel1043 and
RageKnify
|
All tests are now passing in my laptop, still Test262 results need to be checked, and of course, any performance issue. I didn't replace JsString with anything else, since the idea here was to use the interner only in the parsing phase. I added some static strings, the ones being used by us with |
RageKnify
reviewed
Jan 1, 2022
RageKnify
approved these changes
Jan 1, 2022
Razican
force-pushed
the
feature/parser_interner
branch
from
828cf7b
to
e890a8e
Compare
This was referenced Jan 3, 2022
Test262 conformance changesVM implementation
|
Codecov Report
@@ Coverage Diff @@
## main #1765 +/- ##
==========================================
- Coverage 55.72% 55.51% -0.21%
==========================================
Files 201 201
Lines 17336 17420 +84
==========================================
+ Hits 9660 9671 +11
- Misses 7676 7749 +73
Continue to review full report at Codecov.
|
Benchmarks |
raskad
requested changes
Jan 11, 2022
|
I agree with raskad, a |
|
Makes sense, yes, especially now that we have our own interner. In any case, I still see a performance degradation, so maybe We should extend the usage in the compiler/vm and see how it goes |
I actually think the benchmark table is wrong. But the table below seems to switch base and change values: I also ran cargo bench locally for main and this branch and it showed a performance gain for almost all benchmarks. |
raskad
force-pushed
the
feature/parser_interner
branch
from
e890a8e
to
2ff513e
Compare
raskad
approved these changes
Jan 18, 2022
|
@Razican I rebased and implemented the fixes. Imo we can merge this into main. |
Did this add the |
I think the printing order is ok. This is the |
Yep, that's what I meant, it doesn't preserve the original order. It's not critical, but might be something we want. In any case, I think this is good for now. We might want to optimize this by having a specific RegexLiteral node at some point, instead of converting it back to a string. |
Razican
force-pushed
the
feature/parser_interner
branch
from
db3c8c6
to
c4264c5
Compare
|
Benchmarks (properly switching the base):
Now things are looking much, much better |
Razican
force-pushed
the
feature/parser_interner
branch
from
c4264c5
to
94efa38
Compare
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Jan 23, 2022
This builds on top of #1758 to try to bring #1763 to life. Something that should probably be done here would be to convert `JsString` to a `Sym` internally. Then, further optimizations could be done adding common strings to a custom interner type (those that we know statically). This is definitely work in progress, but I would like to have feedback on the API, and feel free to contribute. Co-authored-by: raskad <32105367+raskad@users.noreply.github.com>
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
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.
This builds on top of #1758 to try to bring #1763 to life.
Something that should probably be done here would be to convert
JsStringto aSyminternally. Then, further optimizations could be done adding common strings to a custom interner type (those that we know statically).This is definitely work in progress, but I would like to have feedback on the API, and feel free to contribute.