[Merged by Bors] - Lift InternalObjectMethods from Object
#2790
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
jedel1043
added
Internal
jedel1043
requested review from
Razican,
jasonwilliams,
HalidOdat,
RageKnify,
raskad and
nekevss
jedel1043
changed the title
JsObject internal methods from ObjectInternalObjectMethods from Object
Test262 conformance changes
|
Codecov Report
@@ Coverage Diff @@
## main #2790 +/- ##
==========================================
+ Coverage 51.10% 51.37% +0.26%
==========================================
Files 416 416
Lines 41506 41255 -251
==========================================
- Hits 21213 21195 -18
+ Misses 20293 20060 -233
... and 3 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Benchmark for ed2f008Click to view benchmark
|
Benchmark for 8e418f7Click to view benchmark
|
|
It seems like it slightly improved perf on some tests, but I'll try to run https://bellard.org/quickjs/bench.html to see our current perf score. |
|
Ok, ran QuickJS's benchmarks and got the following results: MainRESULT Richards 29.7 PRRESULT Richards 30.8 So it is safe to say the changes did improve performance. |
|
Both this and #2723 change the |
I think It's fine, since It's one of the things that needs to be refactored so the builtins work well with shapes |
Yeah, I'd also argue that this PR barely changes the object creation API, so it'll be easier to rebase if the smaller change is merged first. |
Benchmark for daf4463Click to view benchmark
|
Benchmark for 3f8b8e8Click to view benchmark
|
Benchmark for 2cf9c0dClick to view benchmark
|
Razican
approved these changes
Apr 12, 2023
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Apr 12, 2023
This Pull Request lifts the `InternalObjectMethods` vtable from `Object`, which should technically improve performance, since we now won't need to call `borrow` to use any of the internal methods, but let's see what the benchmarks show. It changes the following: - Adds a new `VTableObject` struct, containing the old `GcRefCell<Object>` and the lifted `InternalObjectMethods`. - Changes the definition of `JsObject` to `Gc<VTableObject>`. Note that this means the `InternalObjectMethods` are accessible through the `Gc` pointer. - Reestructures intrinsic initialization and initialization APIs to accommodate this change.
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
InternalObjectMethods from ObjectInternalObjectMethods from Object
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 lifts the
InternalObjectMethodsvtable fromObject, which should technically improve performance, since we now won't need to callborrowto use any of the internal methods, but let's see what the benchmarks show.It changes the following:
VTableObjectstruct, containing the oldGcRefCell<Object>and the liftedInternalObjectMethods.JsObjecttoGc<VTableObject>. Note that this means theInternalObjectMethodsare accessible through theGcpointer.