-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
Non recursive gc trace #3508
Non recursive gc trace #3508
Conversation
Test262 conformance changes
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3508 +/- ##
==========================================
- Coverage 49.01% 48.88% -0.13%
==========================================
Files 469 471 +2
Lines 48186 48492 +306
==========================================
+ Hits 23618 23705 +87
- Misses 24568 24787 +219 ☔ View full report in Codecov by Sentry. |
e0c14a2
to
117d950
Compare
Ran the benchmarks and the seems to be a slight speed improvement (not sure why, expected the opposite since we need to reach into the object to be able to trace it). Also just realized that with the queue it would be easier to implement an incremental GC since we can stop at anytime during the tracing and continue afterwards Main
PR
|
117d950
to
4b3feb4
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.
Really nice fix! And yeah, this could simplify a LOT the implementation of a concurrent GC.
Just have some critical comments about some methods of Trace
, and the rest are just nitpicks.
4b3feb4
to
c313927
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.
Good work!
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.
Nice work!
This Pull Request fixes/closes #1848 .
It changes the following:
The reproducible code in the issue has been made into a test for both
boa_gc
andboa_engine
(reduced the chain object from100_000_000
to1_000_000
, takes too long to finish).Note: Marking it as a draft. There are still some things that need to be sorted out. like the vtable extraction and construction without rust-lang/rust#81513 will be much harder to do.Reason for moving vtable to
Gc
object: Because we have<T: Trace + ?Sized> Trace for Gc<T>
we cannot get aNonNull<GcBox<dyn Trace>>
fromGc<dyn Trace>
so we can enque it (we can implement a special case forGc<dyn Trace>
but types likeGc<VTableObject<dyn Trace>>
cannot be done), because it would require us to have double DST, which is not possible. Moving the vtable to theGcBox<T>
allow us to construct aGcBox<dyn Trait>
at any time. This also has the advantage of reducing by half theNonNull<GcBox<dyn Trace>>
pointers in the gc strong vec, also while gc collecting and tracing queue. Could also have some interesting other optimization, since we could have aThinGc<dyn Trace>
that is pointer width while being erased.Solved by create a custom
VTable
with all the required function, and other metadata like size. Being able to put pretty much anything in the vtable, this approach may be more flexible than working with rust-lang/rust#81513.The tests run fine with strict provenance under miri.
Once the warnings, documentation, some code clean up is done, will mark it ready for review.