Rework environment records #1156
Merged
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
|
Is there a way to run the benchmarks on two different commits to compare them with each other and master? I have a different implementation of the fix in |
|
Not automatically but manually you can yes. You can look at the github workflow for benchmarks. It runs a benchmark, checks out another branch (in that case master) then runs the benchmarks again. It starts from here: |
|
Hmm, seems like the actions don't want to run on my fork. Would adding (or removing) commits make the benchmarks run again? I can try rolling the PR back to 1d0d73f so that happens. |
|
You can still view them in https://github.com/boa-dev/boa/pull/1156/checks?check_run_id=2039359835 and check the bottom of the benchmark output. I think for non-members it can't print the results back to this PR @Razican should know more about this issue |
|
Yes, unfortunately, benchmarks only show as comments for project members due to a GitHub limitation, but here we have them: |
0x7D2B
force-pushed
the
lexical-environments
branch
from
33b3634
to
1d0d73f
Compare
|
Ouch, increases all across the board. I wonder if this could be improved at least a little bit by overriding the default methods for creating and getting bindings, given that they do runtime checks for environment types right now? |
|
Benchmarks for iterator implementation: |
|
I noticed a regression in test262 results, the source of which appeared to originate in the test harness. It seems like #989 was masking a different issue with some parts of Boa not cleaning up the environments that they pushed, particularly when returning early. I made some fixes, but I think this needs a closer look. After the fixes, test262 results actually improved: Test262 conformance changes:
Not sure what those two extra panics are, I don't seem to be getting them on my end: |
|
The panics might be some panics that seem to appear and disappear in CI, when it's exactly 2 it can be ok to ignore it. |
Razican
requested review from
jasonwilliams,
Razican,
HalidOdat,
Lan2u,
RageKnify and
tofpie
Razican
added
bug
RageKnify
approved these changes
Mar 16, 2021
Closed
jasonwilliams
approved these changes
Apr 5, 2021
There was a problem hiding this comment.
Hey @0x7D2B thanks so much for taking this on, sorry the review wait was long. Yeah the old system of just iterating through all environments wasn't a great one but got us this far, I'm glad we have something more robust.
|
@0x7D2B I've merged this for now, but I think the e.g https://github.com/boa-dev/boa/blob/master/boa/src/environment/lexical_environment.rs#L154-L168 both of these can be replaced with Issue created #1197 |
Razican
pushed a commit
that referenced
this pull request
May 22, 2021
* Add test for function scope * Make the environment iterator follow outer environments * Make FunctionEnvironmentRecord reuse code * Refactor environment records * Override default methods instead of checking environment types * Fix various environment cleanup issues * Fix lint issues
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 Pull Request fixes/closes #989 by changing the environment traversal logic to use outer environments rather than the lexical environment stack.
This is implemented by moving much of
LexicalEnvironmentlogic intoEnvironmentRecordTraitand making it recursive, allowing for most of the tasks to be implemented without cloning anyGcvalues while still keeping the borrow checker happy.In addition to that,
FunctionEnvironmentRecordhas been updated to useDeclarativeEnvironmentRecordto remove duplicated code. This is similar to howGlobalEnvironmentRecordcurrently works.Finally, an additional test for #989 was added.