[Merged by Bors] - Refactor mapped Arguments object
#1849
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
raskad
added
performance
raskad
requested review from
Razican,
jasonwilliams,
HalidOdat,
RageKnify and
jedel1043
Test262 conformance changesVM implementation
|
Codecov Report
@@ Coverage Diff @@
## main #1849 +/- ##
==========================================
- Coverage 55.57% 55.55% -0.03%
==========================================
Files 199 199
Lines 17756 17768 +12
==========================================
+ Hits 9868 9871 +3
- Misses 7888 7897 +9
Continue to review full report at Codecov.
|
Benchmark for 507f73eClick to view benchmark
|
Razican
approved these changes
Feb 20, 2022
| } | ||
|
|
||
| impl ParameterMap { | ||
| pub(crate) fn delete(&mut self, index: usize) { |
There was a problem hiding this comment.
Suggested change
| pub(crate) fn delete(&mut self, index: usize) { | |
| /// Deletes the binding with the given index from the parameter map. | |
| pub(crate) fn delete(&mut self, index: usize) { |
Comment on lines
30
to
31
| // 10.4.4.7.1 MakeArgGetter ( name, env ) | ||
| // https://tc39.es/ecma262/#sec-makearggetter |
There was a problem hiding this comment.
Could we use documentation comments here?
Comment on lines
39
to
40
| // 10.4.4.7.2 MakeArgSetter ( name, env ) | ||
| // https://tc39.es/ecma262/#sec-makeargsetter |
There was a problem hiding this comment.
Here also, we should use documentation comments, and add a bit of explanation.
Benchmark for 7ee330eClick to view benchmark
|
jedel1043
approved these changes
Feb 21, 2022
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Feb 21, 2022
This refactors the representation of the `[[ParameterMap]]` internal slot on the `Arguments` exotic object to be faster at runtime. Previously `[[ParameterMap]]` was a `JsObject` like the spec describes. This can be pretty slow a runtime, because the argument getters and setters must be represented as function objects on the `[[ParameterMap]]` object. In addition to the time spend on creation and calling of those functions, every getter/setter needs a cloned gc reference to the function environment to access the bindings. This adds to the gc overhead. The spec states that the `[[ParameterMap]]` internal slot doesn't have to be a `JsObject`. See NOTE 3 here: https://tc39.es/ecma262/#sec-arguments-exotic-objects Leveraging this freedom, we can use a more optimized representation, that avoids any `JsObject` usage and only needs one clone of the function environment.
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
Arguments objectArguments object
Razican
pushed a commit
that referenced
this pull request
Feb 21, 2022
This refactors the representation of the `[[ParameterMap]]` internal slot on the `Arguments` exotic object to be faster at runtime. Previously `[[ParameterMap]]` was a `JsObject` like the spec describes. This can be pretty slow a runtime, because the argument getters and setters must be represented as function objects on the `[[ParameterMap]]` object. In addition to the time spend on creation and calling of those functions, every getter/setter needs a cloned gc reference to the function environment to access the bindings. This adds to the gc overhead. The spec states that the `[[ParameterMap]]` internal slot doesn't have to be a `JsObject`. See NOTE 3 here: https://tc39.es/ecma262/#sec-arguments-exotic-objects Leveraging this freedom, we can use a more optimized representation, that avoids any `JsObject` usage and only needs one clone of the function environment.
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 refactors the representation of the
[[ParameterMap]]internal slot on theArgumentsexotic object to be faster at runtime.Previously
[[ParameterMap]]was aJsObjectlike the spec describes. This can be pretty slow a runtime, because the argument getters and setters must be represented as function objects on the[[ParameterMap]]object. In addition to the time spend on creation and calling of those functions, every getter/setter needs a cloned gc reference to the function environment to access the bindings. This adds to the gc overhead.The spec states that the
[[ParameterMap]]internal slot doesn't have to be aJsObject. See NOTE 3 here: https://tc39.es/ecma262/#sec-arguments-exotic-objectsLeveraging this freedom, we can use a more optimized representation, that avoids any
JsObjectusage and only needs one clone of the function environment.