-
-
Notifications
You must be signed in to change notification settings - Fork 413
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement erased objects #3494
Implement erased objects #3494
Conversation
Test262 conformance changes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work! Really like the new design! :)
Just some minor nitpicks and questions.
As for the the threshold thrashing we may want to decrease the used_space_percentage
from 80
to something like 70
(or something simular) rust-gc has is it set to 70
percent.
Ran the benchmarks with that ratio and saw that the speed is more or less the same with main. We can set it to that ratio while we figure out a better way to calculate better thresholds.
Not really worried about coverage since this doesn't add any new builtins. Maybe running the examples would increase our coverage though. |
a56cecf
to
89675dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Just some small nitpicks :)
6c3aa71
to
736f477
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! I also like moving the internal methods to their builtins.
This uses the changes made in #3491 and takes them to the extreme. For a quick rundown, this changes our
current definition of
Object
from:to:
Which allows casting from a specific
Object<T>
to an erasedObject<dyn NativeObject>
, enabling storing them inside aJsObject
.It comes with some very big memory wins overall! Most of these results were generated using small modifications of the Quick JS benchmark.
Heap usage (Main)
dhat: Total: 5,297,772,730 bytes in 42,698,955 blocks
dhat: At t-gmax: 10,021,109 bytes in 68,943 blocks
dhat: At t-end: 8,229,723 bytes in 62,670 blocks
Heap usage (PR)
dhat: Total: 4,344,956,646 bytes in 42,734,151 blocks
dhat: At t-gmax: 9,899,221 bytes in 68,946 blocks
dhat: At t-end: 7,138,579 bytes in 62,072 blocks
Not everything is good news though. I benchmarked the performance after this change and it degraded considerably. I was really confused by this regression, so I profiled a smaller version of the
Splay
benchmark to see the points where the program was spending the most time, but...Main
https://share.firefox.dev/414oQ66
PR
https://share.firefox.dev/3t2vQno
The size wins are so big that the GC is increasing its threshold more slowly, which increases the number of collections overall!
This is extra sad because
Collector::collect
takes 46ms to run on main, while this PR reduces its runtime to 37 ms 😭.If we want to recover the lost performance, we'll need to design more complex heuristics that trigger garbage collection less often, but I would very much prefer to make that change in another PR, because this PR is already pretty massive.
NOTE:
Implement erased objects
is the commit containing the foundation of the PR; the commits succeeding it are mostly patches to migrate our code to the new design.NOTE 2: I'm marking this as draft because it's still missing some changes like documentation or fixing our examples, but I wanted to start the reviews as soon as possible.Done.