8000 Revert "Combine sweeping and moving" · github/ruby@62ce8f9 · GitHub
[go: up one dir, main page]
Skip to content

Commit 62ce8f9

Browse files
tenderlovetenderlove
committed
Revert "Combine sweeping and moving"
This reverts commit 02b216e. This reverts commit 9b8825b. I found that combining sweep and move is not safe. I don't think that we can do compaction concurrently with _anything_ unless there is a read barrier installed. Here is a simple example. A class object is freed, and during it's free step, it tries to remove itself from its parent's subclass list. However, during the sweep step, the parent class was moved and the "currently being freed" class didn't have references updated yet. So we get a segv like this: ``` (lldb) bt * thread #1, name = 'ruby', stop reason = signal SIGSEGV * frame #0: 0x0000560763e344cb ruby`rb_st_lookup at st.c:320:43 frame #1: 0x0000560763e344cb ruby`rb_st_lookup(tab=0x2f7469672f6e6f72, key=3809, value=0x0000560765bf2270) at st.c:1010 frame #2: 0x0000560763e8f16a ruby`rb_search_class_path at variable.c:99:9 frame #3: 0x0000560763e8f141 ruby`rb_search_class_path at variable.c:145 frame #4: 0x0000560763e8f141 ruby`rb_search_class_path(klass=94589785585880) at variable.c:191 frame #5: 0x0000560763ec744e ruby`rb_vm_bugreport at vm_dump.c:996:17 frame #6: 0x0000560763f5b958 ruby`rb_bug_for_fatal_signal at error.c:675:5 frame #7: 0x0000560763e27dad ruby`sigsegv(sig=<unavailable>, info=<unavailable>, ctx=<unavailable>) at signal.c:955:5 frame #8: 0x00007f8b891d33c0 libpthread.so.0`___lldb_unnamed_symbol1$$libpthread.so.0 + 1 frame #9: 0x0000560763efa8bb ruby`rb_class_remove_from_super_subclasses(klass=94589790314280) at class.c:93:56 frame #10: 0x0000560763d10cb7 ruby`gc_sweep_step at gc.c:2674:2 frame #11: 0x0000560763d1187b ruby`gc_sweep at gc.c:4540:2 frame #12: 0x0000560763d101f0 ruby`gc_start at gc.c:6797:6 frame #13: 0x0000560763d15153 ruby`rb_gc_compact at gc.c:7479:12 frame #14: 0x0000560763eb4eb8 ruby`vm_exec_core at vm_insnhelper.c:5183:13 frame #15: 0x0000560763ea9bae ruby`rb_vm_exec at vm.c:1953:22 frame #16: 0x0000560763eac08d ruby`rb_yield at vm.c:1132:9 frame #17: 0x0000560763edb4f2 ruby`rb_ary_collect at array.c:3186:9 frame #18: 0x0000560763e9ee15 ruby`vm_call_cfunc_with_frame at vm_insnhelper.c:2575:12 frame #19: 0x0000560763eb2e66 ruby`vm_exec_core at vm_insnhelper.c:4177:11 frame #20: 0x0000560763ea9bae ruby`rb_vm_exec at vm.c:1953:22 frame #21: 0x0000560763eac08d ruby`rb_yield at vm.c:1132:9 frame #22: 0x0000560763edb4f2 ruby`rb_ary_collect at array.c:3186:9 frame #23: 0x0000560763e9ee15 ruby`vm_call_cfunc_with_frame at vm_insnhelper.c:2575:12 frame #24: 0x0000560763eb2e66 ruby`vm_exec_core at vm_insnhelper.c:4177:11 frame #25: 0x0000560763ea9bae ruby`rb_vm_exec at vm.c:1953:22 frame #26: 0x0000560763ceee01 ruby`rb_ec_exec_node(ec=0x0000560765afa530, n=0x0000560765b088e0) at eval.c:296:2 frame #27: 0x0000560763cf3b7b ruby`ruby_run_node(n=0x0000560765b088e0) at eval.c:354:12 frame #28: 0x0000560763cee4a3 ruby`main(argc=<unavailable>, argv=<unavailable>) at main.c:50:9 frame #29: 0x00007f8b88e560b3 libc.so.6`__libc_start_main + 243 frame #30: 0x0000560763cee4ee ruby`_start + 46 (lldb) f 9 frame #9: 0x0000560763efa8bb ruby`rb_class_remove_from_super_subclasses(klass=94589790314280) at class.c:93:56 90 91 *RCLASS_EXT(klass)->parent_subclasses = entry->next; 92 if (entry->next) { -> 93 RCLASS_EXT(entry->next->klass)->parent_subclasses = RCLASS_EXT(klass)->parent_subclasses; 94 } 95 xfree(entry); 96 } (lldb) command script import -r misc/lldb_cruby.py lldb scripts for ruby has been installed. (lldb) rp entry->next->klass (struct RMoved) $1 = (flags = 30, destination = 94589792806680, next = 94589784369160) (lldb) ```
1 parent b85b866 commit 62ce8f9

File tree

3 files changed

+15
-149
lines changed

3 files changed

+15
-149
lines changed

‎NEWS.md

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -108,14 +108,6 @@ Outstanding ones only.
108108
* Symbol#to_proc now returns a lambda Proc.
109109
[[Feature #16260]]
110110

111-
* GC
112-
113-
* Modified method
114-
115-
* GC.start now takes an option `compact: true` to compact the heap.
116-
For example `GC.start(compact: true)` will have the same effect as
117-
`GC.compact`.
118-
119111
## Stdlib updates
120112

121113
Outstanding ones only.

‎gc.c

Lines changed: 11 additions & 136 deletions
Original file line numberDiff line numberDiff line change
@@ -480,7 +480,6 @@ typedef enum {
480480
GPR_FLAG_HAVE_FINALIZE = 0x4000,
481481
GPR_FLAG_IMMEDIATE_MARK = 0x8000,
482482
GPR_FLAG_FULL_MARK = 0x10000,
483-
GPR_FLAG_COMPACT = 0x20000,
484483

485484
GPR_DEFAULT_REASON =
486485
(GPR_FLAG_FULL_MARK | GPR_FLAG_IMMEDIATE_MARK |
@@ -635,8 +634,6 @@ typedef struct rb_heap_struct {
635634
struct heap_page *using_page;
636635
struct list_head pages;
637636
struct heap_page *sweeping_page; /* iterator for .pages */
638-
struct heap_page *compact_cursor;
639-
VALUE moved_list;
640637
#if GC_ENABLE_INCREMENTAL_MARK
641638
struct heap_page *pooled_pages;
642639
#endif
@@ -670,7 +667,6 @@ typedef struct rb_objspace {
670667
unsigned int gc_stressful: 1;
671668
unsigned int has_hook: 1;
672669
unsigned int during_minor_gc : 1;
673-
unsigned int compact : 1;
674670
#if GC_ENABLE_INCREMENTAL_MARK
675671
unsigned int during_incremental_marking : 1;
676672
#endif
@@ -4182,71 +4178,17 @@ gc_setup_mark_bits(struct heap_page *page)
41824178
memcpy(&page->mark_bits[0], &page->uncollectible_bits[0], HEAP_PAGE_BITMAP_SIZE);
41834179
}
41844180

4185-
static int gc_is_moveable_obj(rb_objspace_t *objspace, VALUE obj);
4186-
static VALUE gc_move(rb_objspace_t *objspace, VALUE scan, VALUE free, VALUE moved_list);
4187-
4188-
static short
4189-
try_move(rb_objspace_t *objspace, rb_heap_t *heap, struct heap_page *sweep_page, VALUE vp)
4190-
{
4191-
struct heap_page * cursor = heap->compact_cursor;
4192-
4193-
while(1) {
4194-
bits_t *mark_bits = cursor->mark_bits;
4195-
RVALUE * p = cursor->start;
4196-
RVALUE * offset = p - NUM_IN_PAGE(p);
4197-
4198-
/* Find an object to move and move it. Movable objects must be
4199-
* marked, so we iterate using the marking bitmap */
4200-
for (size_t i = 0; i < HEAP_PAGE_BITMAP_LIMIT; i++) {
4201-
bits_t bits = mark_bits[i];
4202-
4203-
if (bits) {
4204-
p = offset + i * BITS_BITLENGTH;
4205-
4206-
do {
4207-
if (bits & 1) {
4208-
if (gc_is_moveable_obj(objspace, (VALUE)p)) {
4209-
heap->moved_list = gc_move(objspace, (VALUE)p, vp, heap->moved_list);
4210-
return 1;
4211-
}
4212-
}
4213-
p++;
4214-
bits >>= 1;
4215-
} while (bits);
4216-
}
4217-
}
4218-
4219-
struct heap_page * next;
4220-
4221-
next = list_prev(&heap->pages, cursor, page_node);
4222-
4223-
// Cursors have met, lets quit
4224-
if (next == sweep_page) {
4225-
break;
4226-
} else {
4227-
heap->compact_cursor = next;
4228-
cursor = next;
4229-
}
4230-
}
4231-
4232-
return 0;
4233-
}
4234-
42354181
static inline int
42364182
gc_page_sweep(rb_objspace_t *objspace, rb_heap_t *heap, struct heap_page *sweep_page)
42374183
{
42384184
int i;
4239-
int empty_slots = 0, freed_slots = 0, final_slots = 0, moved_slots = 0;
4185+
int empty_slots = 0, freed_slots = 0, final_slots = 0;
42404186
RVALUE *p, *pend,*offset;
42414187
bits_t *bits, bitset;
42424188

42434189
gc_report(2, objspace, "page_sweep: start.\n");
42444190

42454191
sweep_page->flags.before_sweep = FALSE;
4246-
if (heap->compact_cursor && sweep_page == heap->compact_cursor) {
4247-
/* The compaction cursor and sweep page met, so we need to quit compacting */
4248-
heap->compact_cursor = NULL;
4249-
}
42504192

42514193
p = sweep_page->start; pend = p + sweep_page->total_slots;
42524194
offset = p - NUM_IN_PAGE(p);
@@ -4278,51 +4220,20 @@ gc_page_sweep(rb_objspace_t *objspace, rb_heap_t *heap, struct heap_page *sweep_
42784220
}
42794221
else {
42804222
(void)VALGRIND_MAKE_MEM_UNDEFINED((void*)p, sizeof(RVALUE));
4281-
4282-
if (heap->compact_cursor) {
4283-
/* If try_move couldn't compact anything, it means
4284-
* the cursors have met and there are no objects left that
4285-
* /can/ be compacted, so we need to quit. */
4286-
if (try_move(objspace, heap, sweep_page, vp)) {
4287-
moved_slots++;
4288-
} else {
4289-
heap->compact_cursor = NULL;
4290-
heap_page_add_freeobj(objspace, sweep_page, vp);
4291-
}
4292-
} else {
4293-
heap_page_add_freeobj(objspace, sweep_page, vp);
4294-
}
4295-
4223+
heap_page_add_freeobj(objspace, sweep_page, vp);
42964224
gc_report(3, objspace, "page_sweep: %s is added to freelist\n", obj_info(vp));
4297-
freed_slots++;
4225+
freed_slots++;
4226+
asan_poison_object(vp);
42984227
}
42994228
break;
43004229
}
43014230

43024231
/* minor cases */
4303-
case T_MOVED:
4304-
if (!objspace->flags.during_compacting) {
4305-
/* When compaction is combined with sweeping, some of the swept pages
4306-
* will have T_MOVED objects on them. These need to be kept alive
4307-
* until references are finished updating. Once references are finished
4308-
* updating, `gc_unlink_moved_list` will clear the T_MOVED slots
4309-
* and release them back to the heap. If there are T_MOVED slots
4310-
* in the heap and we're _not_ compacting, then it's a bug. */
4311-
rb_bug("T_MOVED shouldn't be on the heap unless compacting\n");
4312-
}
4313-
break;
43144232
case T_ZOMBIE:
43154233
/* already counted */
43164234
break;
43174235
case T_NONE:
4318-
if (heap->compact_cursor) {
4319-
if (try_move(objspace, heap, sweep_page, vp)) {
4320-
moved_slots++;
4321-
} else {
4322-
heap->compact_cursor = NULL;
4323-
}
4324-
}
4325-
empty_slots++; /* already freed */
4236+
empty_slots++; /* already freed */
43264237
break;
43274238
}
43284239
}
@@ -4346,7 +4257,7 @@ gc_page_sweep(rb_objspace_t *objspace, rb_heap_t *heap, struct heap_page *sweep_
43464257
(int)sweep_page->total_slots,
43474258
freed_slots, empty_slots, final_slots);
43484259

4349-
sweep_page->free_slots = (freed_slots + empty_slots) - moved_slots;
4260+
sweep_page->free_slots = freed_slots + empty_slots;
43504261
objspace->profile.total_freed_objects += freed_slots;
43514262
heap_pages_final_slots += final_slots;
43524263
sweep_page->final_slots += final_slots;
@@ -4360,7 +4271,7 @@ gc_page_sweep(rb_objspace_t *objspace, rb_heap_t *heap, struct heap_page *sweep_
43604271

43614272
gc_report(2, objspace, "page_sweep: end.\n");
43624273

4363-
return (freed_slots + empty_slots) - moved_slots;
4274+
return freed_slots + empty_slots;
43644275
}
43654276

43664277
/* allocate additional minimum page to work */
@@ -4545,29 +4456,6 @@ gc_sweep_continue(rb_objspace_t *objspace, rb_heap_t *heap)
45454456
gc_exit(objspace, "sweep_continue");
45464457
}
45474458

4548-
static void
4549-
gc_compact_start(rb_objspace_t *objspace, rb_heap_t *heap)
4550-
{
4551-
heap->compact_cursor = list_tail(&heap->pages, struct heap_page, page_node);
4552-
objspace->profile.compact_count++;
4553-
heap->moved_list = Qfalse;
4554-
}
4555-
4556-
static void gc_update_references(rb_objspace_t * objspace);
4557-
static void gc_unlink_moved_list(rb_objspace_t *objspace, VALUE moved_list_head);
4558-
4559-
static void
4560-
gc_compact_finish(rb_objspace_t *objspace, rb_heap_t *heap)
4561-
{
4562-
heap->compact_cursor = NULL;
4563-
gc_update_references(objspace);
4564-
rb_clear_constant_cache();
4565-
gc_unlink_moved_list(objspace, heap->moved_list);
4566-
heap->moved_list = Qfalse;
4567-
objspace->flags.compact = 0;
4568-
objspace->flags.during_compacting = FALSE;
4569-
}
4570-
45714459
static void
45724460
gc_sweep(rb_objspace_t *objspace)
45734461
{
@@ -4580,13 +4468,7 @@ gc_sweep(rb_objspace_t *objspace)
45804468
gc_prof_sweep_timer_start(objspace);
45814469
#endif
45824470
gc_sweep_start(objspace);
4583-
if (objspace->flags.compact) {
4584-
gc_compact_start(objspace, heap_eden);
4585-
}
45864471
gc_sweep_rest(objspace);
4587-
if (objspace->flags.compact) {
4588-
gc_compact_finish(objspace, heap_eden);
4589-
}
45904472
#if !GC_ENABLE_LAZY_SWEEP
45914473
gc_prof_sweep_timer_stop(objspace);
45924474
#endif
@@ -7401,8 +7283,6 @@ gc_start(rb_objspace_t *objspace, int reason)
74017283

74027284
/* reason may be clobbered, later, so keep set immediate_sweep here */
74037285
objspace->flags.immediate_sweep = !!((unsigned)reason & GPR_FLAG_IMMEDIATE_SWEEP);
7404-
objspace->flags.compact = !!((unsigned)reason & GPR_FLAG_COMPACT);
7405-
objspace->flags.during_compacting = TRUE;
74067286

74077287
if (!heap_allocated_pages) return FALSE; /* heap is not ready */
74087288
if (!(reason & GPR_FLAG_METHOD) && !ready_to_gc(objspace)) return TRUE; /* GC is not allowed */
@@ -7664,22 +7544,17 @@ garbage_collect_with_gvl(rb_objspace_t *objspace, int reason)
76647544
}
76657545

76667546
static VALUE
7667-
gc_start_internal(rb_execution_context_t *ec, VALUE self, VALUE full_mark, VALUE immediate_mark, VALUE immediate_sweep, VALUE compact)
7547+
gc_start_internal(rb_execution_context_t *ec, VALUE self, VALUE full_mark, VALUE immediate_mark, VALUE immediate_sweep)
76687548
{
76697549
rb_objspace_t *objspace = &rb_objspace;
76707550
int reason = GPR_FLAG_FULL_MARK |
76717551
GPR_FLAG_IMMEDIATE_MARK |
76727552
GPR_FLAG_IMMEDIATE_SWEEP |
76737553
GPR_FLAG_METHOD;
76747554

7675-
/* For now, compact implies full mark / sweep, so ignore other flags */
7676-
if (RTEST(compact)) {
7677-
reason |= GPR_FLAG_COMPACT;
7678-
} else {
7679-
if (!RTEST(full_mark)) reason &= ~GPR_FLAG_FULL_MARK;
7680-
if (!RTEST(immediate_mark)) reason &= ~GPR_FLAG_IMMEDIATE_MARK;
7681-
if (!RTEST(immediate_sweep)) reason &= ~GPR_FLAG_IMMEDIATE_SWEEP;
7682-
}
7555+
if (!RTEST(full_mark)) reason &= ~GPR_FLAG_FULL_MARK;
7556+
if (!RTEST(immediate_mark)) reason &= ~GPR_FLAG_IMMEDIATE_MARK;
7557+
if (!RTEST(immediate_sweep)) reason &= ~GPR_FLAG_IMMEDIATE_SWEEP;
76837558

76847559
garbage_collect(objspace, reason);
76857560
gc_finalize_deferred(objspace);

‎gc.rb

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,17 +26,16 @@ module GC
2626
#
2727
# Use full_mark: false to perform a minor GC.
2828
# Use immediate_sweep: false to defer sweeping (use lazy sweep).
29-
# Use compact: true to compact the heap (it implies a full mark and sweep).
3029
#
3130
# Note: These keyword arguments are implementation and version dependent. They
3231
# are not guaranteed to be future-compatible, and may be ignored if the
3332
# underlying implementation does not support them.
34-
def self.start full_mark: true, immediate_mark: true, immediate_sweep: true, compact: false
35-
__builtin_gc_start_internal full_mark, immediate_mark, immediate_sweep, compact
33+
def self.start full_mark: true, immediate_mark: true, immediate_sweep: true
34+
__builtin_gc_start_internal full_mark, immediate_mark, immediate_sweep
3635
end
3736

3837
def garbage_collect full_mark: true, immediate_mark: true, immediate_sweep: true
39-
__builtin_gc_start_internal full_mark, immediate_mark, immediate_sweep, false
38+
__builtin_gc_start_internal full_mark, immediate_mark, immediate_sweep
4039
end
4140

4241
# call-seq:
@@ -189,7 +188,7 @@ def self.verify_compaction_references(toward: nil, double_heap: false)
189188

190189
module ObjectSpace
191190
def garbage_collect full_mark: true, immediate_mark: true, immediate_sweep: true
192-
__builtin_gc_start_internal full_mark, immediate_mark, immediate_sweep, false
191+
__builtin_gc_start_internal full_mark, immediate_mark, immediate_sweep
193192
end
194193

195194
module_function :garbage_collect

0 commit comments

Comments
 (0)
0