[Merged by Bors] - Refactor the environment for runtime performance #1829
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
Test262 conformance changesVM implementation
Fixed tests (490):Broken tests (80): |
Codecov Report
@@ Coverage Diff @@
## main #1829 +/- ##
==========================================
- Coverage 55.60% 55.57% -0.04%
==========================================
Files 201 199 -2
Lines 17397 17756 +359
==========================================
+ Hits 9674 9868 +194
- Misses 7723 7888 +165
Continue to review full report at Codecov.
|
Benchmark for c004eaaClick to view benchmark
|
raskad
added
execution
Razican
reviewed
Feb 8, 2022
Comment on lines
163
to
168
| // 17. Let mappedNames be a new empty List. | ||
| // using a set to optimize `contains` | ||
| let mut mapped_names = FxHashSet::default(); | ||
|
|
||
| // 12. Let parameterNames be the BoundNames of formals. | ||
| // 13. Let numberOfParameters be the number of elements in parameterNames. | ||
| // 18. Set index to numberOfParameters - 1. | ||
| // 19. Repeat, while index ≥ 0, | ||
| // a. Let name be parameterNames[index]. |
There was a problem hiding this comment.
Could this somehow be mapped to the code? I guess we are changing a bit the algorithm here for performance, right?
There was a problem hiding this comment.
Not really performance. The spec works with identifiers (name) but we do not have identifiers at runtime anymore. We have to work with the binding indices on the environments. The argument bindings are always the first few binding in the environment and we use that to decide what indices the getters and setters should have here.
For example:
function f(a, b, c) {}
f(1,2,3)Looks like this in the function environment:
| binding index | arguments property |
identifier | value |
|---|---|---|---|
| 0 | arguments[0] | a | 1 |
| 1 | arguments[1] | b | 2 |
| 2 | arguments[2] | c | 3 |
This is pretty straightforward, but we can also have this:
function f(a, a, c, c) {}
f(1,2,3)Which translates to:
| binding index | arguments property |
identifier | value |
|---|---|---|---|
| - | arguments[0] | - | - |
| 0 | arguments[1] | a | 2 |
| - | arguments[2] | - | - |
| 1 | arguments[3] | c | undefined |
As you can see, we have to work out the binding index based on the number of arguments and based on the parameter names. The spec assumes, that we can just go forward trough the list of parameter names, but in addition we have to go backwards to find out the binding index (because the last one in the parameter list gets bound).
I think I will add this comment or something similar as documentation to the code to explain how this is different from the spec.
Benchmark for 9b90817Click to view benchmark
|
Benchmark for e60e532Click to view benchmark
|
raskad
requested review from
jasonwilliams,
HalidOdat,
RageKnify,
jedel1043 and
Razican
Razican
reviewed
Feb 12, 2022
boa/src/environments.rs
Outdated
|
|
||
| /// A binding locator contains all information about a binding that is needed to resolve it at runtime. | ||
| /// | ||
| /// Binding locators get created at compile time and are accessible at runtime via the [`CodeBlock`]. |
There was a problem hiding this comment.
It seems that this link to CodeBlock is not working properly.
Benchmark for 05e43dcClick to view benchmark
|
Razican
approved these changes
Feb 13, 2022
| @@ -38,6 +39,9 @@ struct JumpControlInfo { | |||
| kind: JumpControlInfoKind, | |||
| breaks: Vec<Label>, | |||
| try_continues: Vec<Label>, | |||
| in_catch: bool, | |||
There was a problem hiding this comment.
At some point we could add all these booleans as bitflags, should be more efficient.
There was a problem hiding this comment.
I'm not really satisfied with the current exception handling tbh. I will open an issue to think about different ways of how to handle all of that in the vm. I'm hoping that we can find a way to do it without all these fields at runtime.
raskad
force-pushed
the
refactor-environment
branch
from
2d8f4da
to
3929204
Compare
Benchmark for 9a1c7c3Click to view benchmark
|
raskad
force-pushed
the
refactor-environment
branch
from
3929204
to
e303b8b
Compare
Benchmark for 54b6f3bClick to view benchmark
|
RageKnify
reviewed
Feb 19, 2022
RageKnify
reviewed
Feb 19, 2022
RageKnify
reviewed
Feb 19, 2022
RageKnify
reviewed
Feb 19, 2022
RageKnify
reviewed
Feb 19, 2022
Co-authored-by: João Borges <[email protected]>
Benchmark for 8538ec7Click to view benchmark
|
RageKnify
reviewed
Feb 19, 2022
RageKnify
reviewed
Feb 19, 2022
RageKnify
reviewed
Feb 19, 2022
RageKnify
reviewed
Feb 19, 2022
Benchmark for cf6d14fClick to view benchmark
|
RageKnify
approved these changes
Feb 19, 2022
Benchmark for 44a4ad1Click to view benchmark
|
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Feb 19, 2022
This is an attempt to refactor the environments to be more performant at runtime. The idea is, to shift the dynamic hashmap environment lookups from runtime to compile time. Currently the environments hold hashmaps that contain binding identifiers, values and additional information that is needed to identify some errors. Because bindings in outer environments are accessible from inner environments, this can lead to a traversal through all environments (in the worst case to the global environment). This change to the environment structure pushes most of the work that is needed to access bindings to the compile time. At compile time, environments and bindings in the environments are being assigned indices. These indices are then stored instead of the `Sym` that is currently used to access bindings. At runtime, the indices are used to access bindings in a fixed size `Vec` per environment. This brings multiple benefits: - No hashmap access needed at runtime - The number of bindings per environment is known at compile time. Environments only need a single allocation, as their size is constant. - Potential for optimizations with `unsafe` https://doc.rust-lang.org/std/vec/struct.Vec.html#method.get_unchecked Additionally, this changes the global object to have it's bindings directly stored on the `Realm`. This should reduce some overhead from access trough gc objects and makes some optimizations for the global object possible. The benchmarks look not that great on the first sight. But if you look closer, I think it is apparent, that this is a positive change. The difference is most apparent on Mini and Clean as they are longer (still not near any real life js but less specific that most other benchmarks): | Test | Base | PR | % | |------|--------------|------------------|---| | Clean js (Compiler) | **1929.1±5.37ns** | 4.1±0.02µs | **+112.53%** | | Clean js (Execution) | 1487.4±7.50µs | **987.3±3.78µs** | **-33.62%** | The compile time is up in all benchmarks, as expected. The percentage is huge, but if we look at the real numbers, we can see that this is an issue of orders of magnitude. While compile is up `112.53%`, the real change is `~+2µs`. Execution is only down `33.62%`, but the real time changed by `~-500µs`. Co-authored-by: Iban Eguia <[email protected]>
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
Razican
added a commit
that referenced
this pull request
Feb 21, 2022
This is an attempt to refactor the environments to be more performant at runtime. The idea is, to shift the dynamic hashmap environment lookups from runtime to compile time. Currently the environments hold hashmaps that contain binding identifiers, values and additional information that is needed to identify some errors. Because bindings in outer environments are accessible from inner environments, this can lead to a traversal through all environments (in the worst case to the global environment). This change to the environment structure pushes most of the work that is needed to access bindings to the compile time. At compile time, environments and bindings in the environments are being assigned indices. These indices are then stored instead of the `Sym` that is currently used to access bindings. At runtime, the indices are used to access bindings in a fixed size `Vec` per environment. This brings multiple benefits: - No hashmap access needed at runtime - The number of bindings per environment is known at compile time. Environments only need a single allocation, as their size is constant. - Potential for optimizations with `unsafe` https://doc.rust-lang.org/std/vec/struct.Vec.html#method.get_unchecked Additionally, this changes the global object to have it's bindings directly stored on the `Realm`. This should reduce some overhead from access trough gc objects and makes some optimizations for the global object possible. The benchmarks look not that great on the first sight. But if you look closer, I think it is apparent, that this is a positive change. The difference is most apparent on Mini and Clean as they are longer (still not near any real life js but less specific that most other benchmarks): | Test | Base | PR | % | |------|--------------|------------------|---| | Clean js (Compiler) | **1929.1±5.37ns** | 4.1±0.02µs | **+112.53%** | | Clean js (Execution) | 1487.4±7.50µs | **987.3±3.78µs** | **-33.62%** | The compile time is up in all benchmarks, as expected. The percentage is huge, but if we look at the real numbers, we can see that this is an issue of orders of magnitude. While compile is up `112.53%`, the real change is `~+2µs`. Execution is only down `33.62%`, but the real time changed by `~-500µs`. Co-authored-by: Iban Eguia <[email protected]>
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 is an attempt to refactor the environments to be more performant at runtime. The idea is, to shift the dynamic hashmap environment lookups from runtime to compile time.
Currently the environments hold hashmaps that contain binding identifiers, values and additional information that is needed to identify some errors. Because bindings in outer environments are accessible from inner environments, this can lead to a traversal through all environments (in the worst case to the global environment).
This change to the environment structure pushes most of the work that is needed to access bindings to the compile time. At compile time, environments and bindings in the environments are being assigned indices. These indices are then stored instead of the
Symthat is currently used to access bindings. At runtime, the indices are used to access bindings in a fixed sizeVecper environment. This brings multiple benefits:unsafehttps://doc.rust-lang.org/std/vec/struct.Vec.html#method.get_uncheckedAdditionally, this changes the global object to have it's bindings directly stored on the
Realm. This should reduce some overhead from access trough gc objects and makes some optimizations for the global object possible.The benchmarks look not that great on the first sight. But if you look closer, I think it is apparent, that this is a positive change. The difference is most apparent on Mini and Clean as they are longer (still not near any real life js but less specific that most other benchmarks):
The compile time is up in all benchmarks, as expected. The percentage is huge, but if we look at the real numbers, we can see that this is an issue of orders of magnitude. While compile is up
112.53%, the real change is~+2µs. Execution is only down33.62%, but the real time changed by~-500µs.