Skip to content

Move GC sweep page accounting into gc_sweep_page #5691

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Move GC sweep page accounting into gc_sweep_page #5691

wants to merge 2 commits into from

Conversation

eightbitraptor
Copy link
Contributor

This will allow us to use gc_sweep_page outside of the context of gc_sweep_step and maintain the correct accounting of free slots on the page.

@eightbitraptor eightbitraptor force-pushed the mvh-move-heap-page-accounting branch from 0e3968a to 6ecfaee Compare March 22, 2022 21:10
tenderlove
tenderlove reviewed Mar 23, 2022
View reviewed changes
@eightbitraptor eightbitraptor force-pushed the mvh-move-heap-page-accounting branch from 6ecfaee to 2c57dc6 Compare March 24, 2022 10:24
@eightbitraptor eightbitraptor marked this pull request as ready for review March 24, 2022 19:37
@ko1
Copy link
Contributor

ko1 commented Mar 25, 2022

to use gc_sweep_page outside of the context of gc_sweep_step

what is the plan?

@eightbitraptor
Copy link
Contributor Author

@ko1 In this compaction PR #5637 I am using gc_page_sweep to clear out each page as we move the cursor to it ready to fill it. Ideally, I'd use gc_sweep_step so that the accounting is correct and the page gets moved to the tomb at the appropriate time, but I can't because it will either sweep the entire heap, or 2048 slots and I need to sweep a page at a time.

Currently this means that I then have to run an additional sweep once compaction is finished in order to make sure that the accounting is consistent.

If I can move the accounting into gc_sweep_page then I can use it independently of gc_sweep_step and won't have to run a second pass over the heap after compaction has finished.

I hope this makes sense.

@ko1
Copy link
Contributor

ko1 commented Mar 25, 2022

Thank you the explanation. I didn't understand all but it seems make sense.

@eightbitraptor eightbitraptor mentioned this pull request Mar 31, 2022
Merged
@eightbitraptor eightbitraptor marked this pull request as draft April 1, 2022 09:30
@eightbitraptor eightbitraptor force-pushed the mvh-move-heap-page-accounting branch 4 times, most recently from dcc7aaa to fd39f71 Compare April 1, 2022 17:02
@eightbitraptor
Move page sweep cleanup from gc_sweep_step -> gc_sweep_page
0817b92 
This allows gc_sweep_page to be used independently of gc_sweep_step and
will leave the heap in a consistent state.
@eightbitraptor
Only sweep the heap once during compaction
7fa3036
@eightbitraptor eightbitraptor force-pushed the mvh-move-heap-page-accounting branch from fd39f71 to 7fa3036 Compare April 4, 2022 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tenderlove tenderlove tenderlove left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eightbitraptor @ko1 @tenderlove