Skip to content
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

Merged
merged 15 commits into from
Dec 5, 2023
Merged

Implement erased objects #3494

merged 15 commits into from
Dec 5, 2023

Conversation

jedel1043
Copy link
Member

@jedel1043 jedel1043 commented Dec 3, 2023

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:

pub struct Object {
    kind: ObjectKind,
    properties: PropertyMap,
    extensible: bool,
    private_elements: ThinVec<(PrivateName, PrivateElement)>,
}

to:

pub struct Object<T: ?Sized> {
    pub(crate) properties: PropertyMap,
    pub(crate) extensible: bool,
    private_elements: ThinVec<(PrivateName, PrivateElement)>,
    data: T,
}

Which allows casting from a specific Object<T> to an erased Object<dyn NativeObject>, enabling storing them inside a JsObject.

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

image

PR

https://share.firefox.dev/3t2vQno

image

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.

@jedel1043 jedel1043 added performance Performance related changes and issues builtins PRs and Issues related to builtins/intrinsics API labels Dec 3, 2023
@jedel1043 jedel1043 added this to the v0.18.0 milestone Dec 3, 2023
@jedel1043 jedel1043 requested a review from a team December 3, 2023 10:53
Copy link

github-actions bot commented Dec 3, 2023

Test262 conformance changes

Test result main count PR count difference
Total 95,609 95,609 0
Passed 76,526 76,526 0
Ignored 18,132 18,132 0
Failed 951 951 0
Panics 0 0 0
Conformance 80.04% 80.04% 0.00%

Copy link
Member

@HalidOdat HalidOdat left a 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.

boa_engine/src/builtins/proxy/mod.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/proxy/mod.rs Outdated Show resolved Hide resolved
boa_engine/src/builtins/object/mod.rs Show resolved Hide resolved
@HalidOdat HalidOdat requested a review from a team December 3, 2023 16:17
Copy link

codecov bot commented Dec 3, 2023

Codecov Report

Attention: 1454 lines in your changes are missing coverage. Please review.

Comparison is base (c113b74) 46.42% compared to head (eca8048) 46.83%.

Files Patch % Lines
boa_engine/src/builtins/proxy/mod.rs 5.05% 338 Missing ⚠️
...src/builtins/typed_array/integer_indexed_object.rs 0.00% 137 Missing ⚠️
boa_engine/src/builtins/temporal/calendar/mod.rs 21.05% 135 Missing ⚠️
boa_engine/src/builtins/typed_array/builtin.rs 0.00% 97 Missing ⚠️
boa_engine/src/object/mod.rs 27.17% 67 Missing ⚠️
boa_engine/src/builtins/function/arguments.rs 28.20% 56 Missing ⚠️
boa_engine/src/builtins/function/bound.rs 0.00% 54 Missing ⚠️
boa_engine/src/builtins/temporal/instant/mod.rs 0.00% 45 Missing ⚠️
boa_engine/src/value/display.rs 57.83% 35 Missing ⚠️
boa_macros/src/lib.rs 0.00% 28 Missing ⚠️
... and 69 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3494      +/-   ##
==========================================
+ Coverage   46.42%   46.83%   +0.41%     
==========================================
  Files         496      493       -3     
  Lines       50775    50077     -698     
==========================================
- Hits        23570    23455     -115     
+ Misses      27205    26622     -583     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jedel1043
Copy link
Member Author

Not really worried about coverage since this doesn't add any new builtins. Maybe running the examples would increase our coverage though.

@jedel1043 jedel1043 marked this pull request as ready for review December 3, 2023 21:02
Copy link
Member

@HalidOdat HalidOdat left a 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 :)

boa_engine/src/builtins/set/set_iterator.rs Show resolved Hide resolved
boa_engine/src/builtins/object/mod.rs Show resolved Hide resolved
boa_engine/src/object/jsobject.rs Outdated Show resolved Hide resolved
@HalidOdat HalidOdat requested a review from a team December 4, 2023 06:37
Copy link
Member

@raskad raskad left a 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.

@raskad raskad added this pull request to the merge queue Dec 5, 2023
Merged via the queue into main with commit 055e8fe Dec 5, 2023
14 checks passed
@jedel1043 jedel1043 deleted the erased-objects branch December 5, 2023 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API builtins PRs and Issues related to builtins/intrinsics performance Performance related changes and issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants