Skip to content

Conversation

@qinsoon
Copy link
Member

@qinsoon qinsoon commented Mar 25, 2025

This PR moves the call to aggregate_live_bytes_in_last_gc from the Release work packet to on_gc_finished. Spaces may do release work in parallel with the Release work packet, and aggregate_live_bytes_in_last_gc accesses reserved pages of spaces. This leads to a data race. We observed the reserved pages in the collected live bytes stats to be larger than the actual reserved pages at the end of a GC, as some pages were not released yet.

@qinsoon qinsoon requested a review from wks March 27, 2025 23:46
Comment on lines 562 to 565
let mut stats = mmtk.state.live_bytes_in_last_gc.borrow_mut();
*stats = mmtk.aggregate_live_bytes_in_last_gc(live_bytes);
// Logging
for (space_name, &stats) in stats.iter() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are two different variables with the same name stats, one defined on line 562 by the let mut, and the other on line 565 by the &stats in the for loop. It is a bit confusing. I suggest renaming the former to live_bytes_in_last_gc, map or guard, whatever makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for spotting this.

Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

LGTM

@qinsoon qinsoon added this pull request to the merge queue Mar 30, 2025
Merged via the queue into mmtk:master with commit ce0ba41 Mar 30, 2025
33 checks passed
@qinsoon qinsoon deleted the aggregate-live-bytes-in-end-of-gc branch March 30, 2025 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants