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

Find roots when running GC rather than runtime #3109

Merged
merged 9 commits into from
Jul 26, 2023

Conversation

tunz
Copy link
Contributor

@tunz tunz commented Jul 4, 2023

Attempt to address the issue #2773.

The existing implementation had an expensive overhead of managing root counts, especially for mutable borrow of GcRefCell. Instead of managing the root counts, this change counts the number of Gc/WeakGc handles located in Gc heap objects and total number of them. Then, we can find whether there is a root by comparing those numbers.

For example, in the following case, there are 2 Gcs for Gc<i32> type. One is allocated on the stack, and the other one is located in another Gc heap object.

#[derive(Trace, Finalize)]
struct Obj {
  Gc<i32> v,
}
let num = Gc::new(123);
let v = Gc::new(Obj { v: num.clone() });

Then, we'll iterate and trace the two objects when running GC, and mark the referenced Gcs as non root. We know that there are 2 Gc, but only one of them is marked as non-root. So, we can find that the remaining one is the root.

As a trade-off, this approach has an overhead in the the garbage collection time, but I believe there is a room for improvement such as tracing non roots in parallel.

Here is a result of benchmarks:

Richards: 25.5 -> 26.4
DeltaBlue: 28.7 -> 29.1
Crypto: 33.4 -> 38.0
RayTrace: 101 -> 107
EarleyBoyer: 88.3 -> 95.3
Splay: 104 -> 93.2
NavierStokes: 9.94 -> 75.3
---
SCORE: 41.9 -> 57.5

There is one regression in Splay benchmark because it has a lot of DeclarativeEnvironment allocation triggering GC, so it's more impacted by the overhead of GC than benefit of faster property accesses.

Attempt to address the issue boa-dev#2773.

The existing implementation had an expensive overhead of managing root
counts, especially for mutable borrow of GcRefCell.

Instead of managing the root counts, this change counts the number of
Gc/WeakGc handles located in Gc heap objects and total number of them.
Then, we can find whether there is a root by comparing those numbers.
@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Merging #3109 (d8a59fe) into main (e97d1f6) will decrease coverage by 0.03%.
The diff coverage is 91.73%.

@@            Coverage Diff             @@
##             main    #3109      +/-   ##
==========================================
- Coverage   50.61%   50.58%   -0.03%     
==========================================
  Files         444      443       -1     
  Lines       42696    42641      -55     
==========================================
- Hits        21612    21572      -40     
+ Misses      21084    21069      -15     
Impacted Files Coverage Δ
boa_engine/src/lib.rs 74.02% <ø> (ø)
boa_engine/src/native_function.rs 68.00% <ø> (-2.38%) ⬇️
boa_gc/src/pointers/weak.rs 70.58% <ø> (ø)
boa_gc/src/pointers/weak_map.rs 80.00% <ø> (ø)
boa_macros/src/lib.rs 0.00% <0.00%> (ø)
boa_engine/src/builtins/array/mod.rs 76.93% <66.66%> (-0.21%) ⬇️
boa_gc/src/internals/gc_box.rs 88.23% <90.32%> (+1.05%) ⬆️
boa_gc/src/internals/ephemeron_box.rs 85.91% <90.62%> (+0.91%) ⬆️
boa_gc/src/cell.rs 47.40% <100.00%> (-1.96%) ⬇️
boa_gc/src/lib.rs 99.45% <100.00%> (+0.04%) ⬆️
... and 3 more

@HalidOdat HalidOdat requested a review from a team July 5, 2023 10:37
@HalidOdat HalidOdat added performance Performance related changes and issues gc Issue related to garbage collection labels Jul 5, 2023
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really liking the big performance wins! However, I have some suggestions about the design.

boa_gc/src/pointers/gc.rs Outdated Show resolved Hide resolved
@tunz
Copy link
Contributor Author

tunz commented Jul 20, 2023

There are too many must_use clippy errors in boa_engine. I guess something blocking clippy is removed. So I disabled it for now. Please let me know if we want to fix them all in this PR.

/// - Mark Flag Bit
///
/// The next node is set by the `Allocator` during initialization and by the
/// `Collector` during the sweep phase.
pub(crate) struct GcBoxHeader {
roots: Cell<usize>,
marked: Cell<bool>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thoughts on using a bit in the ref_count here over a full bool?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hesitant to keep the mark bit in ref_count since there was 1~2% overhead, but it seems like putting the mark bit into non_root_count does not have that much of overhead. Updated to include the mark bit in non_root_count.

Copy link
Member

@jedel1043 jedel1043 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! We can resolve the lints on a follow-up PR :)

@jedel1043 jedel1043 requested review from Razican and a team and removed request for Razican July 22, 2023 05: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.

Great work @tunz !

@HalidOdat HalidOdat added this pull request to the merge queue Jul 26, 2023
Merged via the queue into boa-dev:main with commit 6a78629 Jul 26, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gc Issue related to garbage collection performance Performance related changes and issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants