Stop double sweeping the heap during compaction #5757
Conversation
7fa3036 to
cbd98ff
Compare
| int unlink_pages = GC_SWEEP_PAGES_FREEABLE_PER_STEP; | ||
| int swept_slots = 0; | ||
| gc_sweep_page(objspace, pool, dheap->sweeping_page, &unlink_pages, &swept_slots, FALSE); |
There was a problem hiding this comment.
This is always going to move the page to the tomb heap if the page is empty because unlink_pages > 0 is always true.
There was a problem hiding this comment.
yes, good spot, thanks
| @@ -5481,6 +5492,46 @@ gc_sweep_page(rb_objspace_t *objspace, rb_heap_t *heap, struct gc_sweep_context | |||
| gc_report(2, objspace, "page_sweep: end.\n"); | |||
| } | |||
|
|
|||
| static void | |||
| gc_sweep_page(rb_objspace_t *objspace, rb_size_pool_t *size_pool, struct heap_page *sweep_page, int *unlink_limit, int *swept_slots, bool need_pool) | |||
There was a problem hiding this comment.
I don't think gc_sweep_page should take care of moving pages to pooled pages or to the tomb heap. For compaction, we shouldn't need to do either of the two, so I think the logic here should stay in gc_sweep_step.
There was a problem hiding this comment.
Thanks Peter - I've had a go at teasing apart some of these conditionals so now gc_sweep_page is responsible for adding the page back to the free_pages where needed, but gc_sweep_step retains the responsibility for adding the page to the tomb, and the pool pages if necessary. What do you think?
cbd98ff to
d23132c
Compare
This allows gc_sweep_page to be used independently of gc_sweep_step and will leave the heap in a consistent state.
d23132c to
69222b0
Compare
In #5637 I reversed the order of steps in the compaction algorithm. This meant changing the way pages are swept during compaction, however because of how
gc_sweep_stepandgc_sweep_pageinteract we still required a seperate sweep step after compaction had finished, to finalise the accounting and the free_pages/pool_pages lists.This PR refactors
gc_sweep_stepandgc_sweep_pageto make them more independent -gc_sweep_pagenow handles adding the page to the free/pool pages and incrementing the sweeping_page cursor - this is so that we can usegc_sweep_pagedirectly during compaction to fully sweep a page and maintain correct accounting.This has allowed us to change the entry point for compaction to a branch - either sweep and compact, or just sweep - removing the need for a seperate sweep step post compaction.