[Merged by Bors] - Implement Hidden classes
#2723
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
HalidOdat
added
execution
|
😮😮😮 |
Codecov Report
@@ Coverage Diff @@
## main #2723 +/- ##
==========================================
+ Coverage 50.95% 50.97% +0.01%
==========================================
Files 419 423 +4
Lines 41940 41751 -189
==========================================
- Hits 21372 21281 -91
+ Misses 20568 20470 -98
... and 101 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. |
Test262 conformance changes
New panics (1): |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
HalidOdat
force-pushed
the
optimization/object-shapes
branch
from
ef12e35
to
f91d43a
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
HalidOdat
force-pushed
the
optimization/object-shapes
branch
from
f91d43a
to
42fe677
Compare
This comment was marked as outdated.
This comment was marked as outdated.
HalidOdat
force-pushed
the
optimization/object-shapes
branch
from
6c93dac
to
87613fe
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
HalidOdat
force-pushed
the
optimization/object-shapes
branch
from
8f3dbb6
to
4ea5ada
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Benchmark for 7e63a54Click to view benchmark
|
HalidOdat
force-pushed
the
optimization/object-shapes
branch
from
629a35a
to
2ebee01
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Benchmark for 759ebd2Click to view benchmark
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
This is now nearing completion :) Still have some minor things left! But I think this is ready for review now! Added code documentation as well as a markdown The benchmarks are not looking good, shapes on their own are more of a memory optimization, will be working on inline caching, which should speed up the execution. |
Benchmark for 9a1cc45Click to view benchmark
|
|
Currently the integration with builtin construction is not fully complete, there are still some places that don't utilize direct shape initialization, but I have left it for #2813 which should improve buitin construction! :) |
|
I still want to take a second review of this also |
HalidOdat
force-pushed
the
optimization/object-shapes
branch
from
c71d765
to
bf6681f
Compare
Benchmark for b066366Click to view benchmark
|
|
Did some benchmarks on MainShapesShapes + Inline CachingWe can see a pretty big increase in performance, interestingly even just shapes has a big performance increase without inline caching. |
HalidOdat
force-pushed
the
optimization/object-shapes
branch
from
bf6681f
to
715ef73
Compare
Benchmark for e2cf294Click to view benchmark
|
HalidOdat
force-pushed
the
optimization/object-shapes
branch
from
715ef73
to
be6ad76
Compare
Benchmark for 725bbb8Click to view benchmark
|
HalidOdat
force-pushed
the
optimization/object-shapes
branch
from
be6ad76
to
bd062f8
Compare
Benchmark for 401047eClick to view benchmark
|
Any idea why this one has gone up so much? |
I suspect that is because the test is basically just adding many index properties to an array while creating the array literal. This process is now creating the relevant shapes which is where the overhead comes from. |
|
Benchmark: (function () {
let testArray = [ /* Many elements */ ];
while (testArray.length > 0) {
testArray.pop();
}
return testArray;
})();
Indexed properties are special as they don't cause a shape transition, they are stored in My guess is that it's the layers of indirection, EDIT: Ran the benchmarks locally on |
jasonwilliams
approved these changes
Apr 25, 2023
|
Ok thanks for looking, the rest of the PR looks fine to me, good job! |
- Make forward reference weak gc - Add shape property table cache - Implement unique shape - Implement unique shape property deleting delete - Move property desciptor flags to shape - Remove unneeded Option in property_table - Implement shared property table - Remove unneeded return of insert and remove - Implement prototype transitions - Implement varying width property storage - Convert shape to unique if shape has exceeded transition max - Add documentation - Add `shape.md` documentation - Use FxHashMap for lookup and vec for keys - Direct initialization of arrays and objects with shape - Make functions share the same shape - Apply shared shapes to builtins - Add forward reference check for attribute change - Direct initialization of arrays - Remove overwritten length property on TypedArray - Refactor templates into separate struct - Apply shapes to callable builtins - Initialize builtin constructors directly - Add inline caching to constructor function result
HalidOdat
force-pushed
the
optimization/object-shapes
branch
from
bd062f8
to
a62710c
Compare
Benchmark for 29ecc0fClick to view benchmark
|
|
The part that I reviewed looked great. Amazing work!! bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Apr 26, 2023
This PR implements `Hidden Classes`, I named them as `Shapes` (like Spidermonkey does), calling them maps like v8 seems confusing because there already is a JS builtin, likewise with `Hidden classes` since there are already classes in JS.
There are two types of shapes: `shared` shapes that create the transition tree, and are shared between objects, this is mainly intended for user defined objects this makes more sense because shapes can create transitions trees, doing that for the builtins seems wasteful (unless users wanted to creating an object with the same property names and the same property attributes in the same order... which seems unlikely). That's why I added `unique` shapes, only one object has it. This is similar to previous solution, but this architecture enables us to use inline caching.
There will probably be a performance hit until we implement inline caching.
There still a lot of work that needs to be done, on this:
- [x] Move Property Attributes to shape
- [x] Move Prototype to shape
- [x] ~~Move extensible flag to shape~~, On further evaluation this doesn't give any benefit (at least right now), since it isn't used by inline caching also adding one more transition.
- [x] Implement delete for unique shapes.
- [x] If the chain is too long we should probably convert it into a `unique` shape
- [x] Figure out threshold ~~(maybe more that 256 properties ?)~~ curently set to an arbitrary number (`1024`)
- [x] Implement shared property table between shared shapes
- [x] Add code Document
- [x] Varying size storage for properties (get+set = 2, data = 1)
- [x] Add shapes to more object:
- [x] ordinary object
- [x] Arrays
- [x] Functions
- [x] Other builtins
- [x] Add `shapes.md` doc explaining shapes in depth with mermaid diagrams :)
- [x] Add `$boa.shape` module
- [x] `$boa.shape.id(o)`
- [x] `$boa.shape.type(o)`
- [x] `$boa.shape.same(o1, o2)`
- [x] add doc to `boa_object.md`
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
Hidden classesHidden classes
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 PR implements
Hidden Classes, I named them asShapes(like Spidermonkey does), calling them maps like v8 seems confusing because there already is a JS builtin, likewise withHidden classessince there are already classes in JS.There are two types of shapes:
sharedshapes that create the transition tree, and are shared between objects, this is mainly intended for user defined objects this makes more sense because shapes can create transitions trees, doing that for the builtins seems wasteful (unless users wanted to creating an object with the same property names and the same property attributes in the same order... which seems unlikely). That's why I addeduniqueshapes, only one object has it. This is similar to previous solution, but this architecture enables us to use inline caching.There will probably be a performance hit until we implement inline caching.
There still a lot of work that needs to be done, on this:
Move extensible flag to shape, On further evaluation this doesn't give any benefit (at least right now), since it isn't used by inline caching also adding one more transition.uniqueshape(maybe more that 256 properties ?)curently set to an arbitrary number (1024)shapes.mddoc explaining shapes in depth with mermaid diagrams :)$boa.shapemodule$boa.shape.id(o)$boa.shape.type(o)$boa.shape.same(o1, o2)boa_object.md