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

[Merged by Bors] - Improve the design of ephemerons in our GC #2530

Closed
wants to merge 6 commits into from

Conversation

jedel1043
Copy link
Member

This PR changes the following:

  • Modifies EphemeronBox to be more akin to GcBox, with its own header, roots and markers. This also makes it more similar to Racket's implementation.
  • Removes EPHEMERON_QUEUE.
  • Ephemerons are now tracked on a special weak_start linked list, instead of strong_start which is where all other GC boxes live.
  • Documents all unsafe blocks.
  • Documents our current garbage collection algorithm. I hope this'll clarify a bit what exactly are we doing on every garbage collection.
  • Renames/removes some functions.

@jedel1043 jedel1043 added technical debt execution Issues or PRs related to code execution labels Jan 13, 2023
@jedel1043 jedel1043 added this to the v0.17.0 milestone Jan 13, 2023
@github-actions
Copy link

github-actions bot commented Jan 13, 2023

Test262 conformance changes

Test result main count PR count difference
Total 94,205 94,205 0
Passed 70,624 70,624 0
Ignored 18,622 18,622 0
Failed 4,959 4,959 0
Panics 0 0 0
Conformance 74.97% 74.97% 0.00%

@codecov
Copy link

codecov bot commented Jan 13, 2023

Codecov Report

Merging #2530 (211f150) into main (f19467a) will increase coverage by 0.16%.
The diff coverage is 88.25%.

@@            Coverage Diff             @@
##             main    #2530      +/-   ##
==========================================
+ Coverage   49.99%   50.15%   +0.16%     
==========================================
  Files         379      379              
  Lines       37642    37660      +18     
==========================================
+ Hits        18819    18889      +70     
+ Misses      18823    18771      -52     
Impacted Files Coverage Δ
boa_engine/src/builtins/async_generator/mod.rs 8.00% <ø> (ø)
boa_engine/src/builtins/generator/mod.rs 15.00% <ø> (ø)
boa_engine/src/bytecompiler/mod.rs 58.10% <ø> (ø)
boa_engine/src/vm/code_block.rs 30.60% <0.00%> (ø)
boa_engine/src/vm/opcode/await_stm/mod.rs 0.00% <0.00%> (ø)
boa_examples/src/bin/closures.rs 0.00% <0.00%> (ø)
boa_gc/src/trace.rs 65.00% <ø> (+5.00%) ⬆️
boa_macros/src/lib.rs 0.00% <ø> (ø)
boa_engine/src/builtins/promise/mod.rs 23.33% <40.00%> (ø)
boa_gc/src/cell.rs 57.80% <53.84%> (-1.52%) ⬇️
... and 17 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@nekevss nekevss left a comment

Choose a reason for hiding this comment

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

I'm going to try and look at this in more depth and play around with it this weekend, but I wanted to leave some early comments.

On the topic of improvements/clarity changes, I was thinking about the current names of some structs. In particular, thoughts on switching GcCell to GcRefCell, GcCellRef to GcRef, and GcCellRefMut to GcRefMut?

@jedel1043
Copy link
Member Author

thoughts on switching GcCell to GcRefCell, GcCellRef to GcRef, and GcCellRefMut to GcRefMut?

I like it! The current names make GcCell look like it has the semantics of Cell but garbage collected, which is definitely not true. I'll rename them.

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.

Nice changes. I like the renaming too.

boa_gc/src/lib.rs Show resolved Hide resolved
Copy link
Member

@nekevss nekevss left a comment

Choose a reason for hiding this comment

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

Looks like it should be fine to move forward with. Nice work on the improvements. 😄

@jedel1043
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Jan 23, 2023
This PR changes the following:
- Modifies `EphemeronBox` to be more akin to `GcBox`, with its own header, roots and markers. This also makes it more similar to [Racket's](https://docs.racket-lang.org/reference/ephemerons.html) implementation.
- Removes `EPHEMERON_QUEUE`.
- Ephemerons are now tracked on a special `weak_start` linked list, instead of `strong_start` which is where all other GC boxes live.
- Documents all unsafe blocks.
- Documents our current garbage collection algorithm. I hope this'll clarify a bit what exactly are we doing on every garbage collection.
- Renames/removes some functions.
@bors
Copy link

bors bot commented Jan 23, 2023

Build failed:

@jedel1043
Copy link
Member Author

bors retry

bors bot pushed a commit that referenced this pull request Jan 23, 2023
This PR changes the following:
- Modifies `EphemeronBox` to be more akin to `GcBox`, with its own header, roots and markers. This also makes it more similar to [Racket's](https://docs.racket-lang.org/reference/ephemerons.html) implementation.
- Removes `EPHEMERON_QUEUE`.
- Ephemerons are now tracked on a special `weak_start` linked list, instead of `strong_start` which is where all other GC boxes live.
- Documents all unsafe blocks.
- Documents our current garbage collection algorithm. I hope this'll clarify a bit what exactly are we doing on every garbage collection.
- Renames/removes some functions.
@bors
Copy link

bors bot commented Jan 23, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Improve the design of ephemerons in our GC [Merged by Bors] - Improve the design of ephemerons in our GC Jan 23, 2023
@bors bors bot closed this Jan 23, 2023
@bors bors bot deleted the gc-improvements branch January 23, 2023 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
execution Issues or PRs related to code execution technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants