From 919b6adae4a2227a69cb7066997fc1930473a7bc Mon Sep 17 00:00:00 2001 From: gbaraldi Date: Fri, 10 May 2024 13:17:49 -0300 Subject: [PATCH 1/6] Always root the contents in expression, regardless if they are top level or not. --- src/codegen.cpp | 3 +-- test/gc.jl | 13 +++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/codegen.cpp b/src/codegen.cpp index 6d4ecc63e5ca1..8a163bc3cee99 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -6378,8 +6378,7 @@ static jl_cgval_t emit_expr(jl_codectx_t &ctx, jl_value_t *expr, ssize_t ssaidx_ jl_value_t *val = expr; if (jl_is_quotenode(expr)) val = jl_fieldref_noalloc(expr, 0); - if (jl_is_method(ctx.linfo->def.method)) // toplevel exprs are already rooted - val = jl_ensure_rooted(ctx, val); + val = jl_ensure_rooted(ctx, val); return mark_julia_const(ctx, val); } diff --git a/test/gc.jl b/test/gc.jl index f924f4952cfb0..b136e556aa020 100644 --- a/test/gc.jl +++ b/test/gc.jl @@ -72,3 +72,16 @@ end @testset "Base.GC docstrings" begin @test isempty(Docs.undocumented_names(GC)) end + +#testset doesn't work here because this needs to run in top level +#Check that we ensure objects in toplevel exprs are rooted +global dims54422 = [] # allocate the Binding +GC.gc(); GC.gc(); # force the binding to be old +GC.enable(false); # prevent new objects from being old +@eval begin + Base.Experimental.@force_compile # use the compiler + dims54422 = $([]) + nothing +end +GC.enable(true); GC.gc(false) # incremental collection +@test dims54422 == [] From 2b2420c39f64e97abf79f821ba62ef8038d6e31b Mon Sep 17 00:00:00 2001 From: gbaraldi Date: Fri, 10 May 2024 14:32:05 -0300 Subject: [PATCH 2/6] Update tests slightly --- test/gc.jl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/gc.jl b/test/gc.jl index b136e556aa020..e46ff0ed73fd9 100644 --- a/test/gc.jl +++ b/test/gc.jl @@ -84,4 +84,5 @@ GC.enable(false); # prevent new objects from being old nothing end GC.enable(true); GC.gc(false) # incremental collection -@test dims54422 == [] +@test typeof(dims54422) == Vector{Any} +@test isempty(dims54422) From c392082e034df58a325c9b8341668b26bc88e0dd Mon Sep 17 00:00:00 2001 From: gbaraldi Date: Fri, 17 May 2024 10:49:14 -0300 Subject: [PATCH 3/6] Instead of permanently rooting, make the global and marked + in the remset. --- src/codegen.cpp | 8 +++++++- src/gc-stock.c | 30 ++++++++++++++++++++++++++++++ src/jl_exported_funcs.inc | 1 + src/julia_internal.h | 1 + 4 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/codegen.cpp b/src/codegen.cpp index 8a163bc3cee99..0811826ea6f4d 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -6378,7 +6378,13 @@ static jl_cgval_t emit_expr(jl_codectx_t &ctx, jl_value_t *expr, ssize_t ssaidx_ jl_value_t *val = expr; if (jl_is_quotenode(expr)) val = jl_fieldref_noalloc(expr, 0); - val = jl_ensure_rooted(ctx, val); + if (jl_is_method(ctx.linfo->def.method)) // toplevel exprs are already rooted + val = jl_ensure_rooted(ctx, val); + else { + jl_task_t *ct = jl_current_task; + jl_gc_force_mark_old(ct->ptls, val); // Make the value old and marked + put it in the remset + } // Top level expressions are no longer rooted after execution + // so storing a new object into an old binding will cause GC corruption return mark_julia_const(ctx, val); } diff --git a/src/gc-stock.c b/src/gc-stock.c index d25f8917f302d..ab2ae8e85d933 100644 --- a/src/gc-stock.c +++ b/src/gc-stock.c @@ -4013,6 +4013,36 @@ JL_DLLEXPORT void jl_gc_schedule_foreign_sweepfunc(jl_ptls_t ptls, jl_value_t *o arraylist_push(&ptls->gc_tls.sweep_objs, obj); } +void jl_gc_force_mark_old(jl_ptls_t ptls, jl_value_t *v) JL_NOTSAFEPOINT +{ + jl_taggedvalue_t *o = jl_astaggedvalue(v); + jl_datatype_t *dt = (jl_datatype_t*)jl_typeof(v); + size_t dtsz = jl_datatype_size(dt); + if (o->bits.gc == GC_OLD_MARKED) + return; + o->bits.gc = GC_OLD_MARKED; + if (dt == jl_simplevector_type) { + size_t l = jl_svec_len(v); + dtsz = l * sizeof(void*) + sizeof(jl_svec_t); + } + else if (dt->name == jl_genericmemory_typename) { + dtsz = sizeof(jl_genericmemory_t); + } + else if (dt == jl_module_type) { + dtsz = sizeof(jl_module_t); + } + else if (dt == jl_task_type) { + dtsz = sizeof(jl_task_t); + } + else if (dt == jl_symbol_type) { + return; + } + gc_setmark(ptls, o, GC_OLD_MARKED, dtsz); + if (dt->layout->npointers != 0) + jl_gc_queue_root(v); +} + + #ifdef __cplusplus } #endif diff --git a/src/jl_exported_funcs.inc b/src/jl_exported_funcs.inc index 7abf2b055bb8c..aa12725282034 100644 --- a/src/jl_exported_funcs.inc +++ b/src/jl_exported_funcs.inc @@ -170,6 +170,7 @@ XX(jl_gc_small_alloc) \ XX(jl_gc_queue_multiroot) \ XX(jl_gc_queue_root) \ + XX(jl_gc_force_mark_old) \ XX(jl_gc_safepoint) \ XX(jl_gc_schedule_foreign_sweepfunc) \ XX(jl_gc_set_cb_notify_external_alloc) \ diff --git a/src/julia_internal.h b/src/julia_internal.h index f00667d016796..fc3d79c3047ab 100644 --- a/src/julia_internal.h +++ b/src/julia_internal.h @@ -374,6 +374,7 @@ jl_value_t *jl_gc_small_alloc_noinline(jl_ptls_t ptls, int offset, int osize); jl_value_t *jl_gc_big_alloc_noinline(jl_ptls_t ptls, size_t allocsz); JL_DLLEXPORT int jl_gc_classify_pools(size_t sz, int *osize) JL_NOTSAFEPOINT; +JL_DLLEXPORT void jl_gc_force_mark_old(jl_ptls_t ptls, jl_value_t *v); void gc_sweep_sysimg(void); From 0239e3332b558626a573c80dac12a7788eb97376 Mon Sep 17 00:00:00 2001 From: gbaraldi Date: Fri, 17 May 2024 11:26:16 -0300 Subject: [PATCH 4/6] gc_setmark should check if the object belongs to a image --- src/gc-stock.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/gc-stock.c b/src/gc-stock.c index ab2ae8e85d933..919219eb31f5d 100644 --- a/src/gc-stock.c +++ b/src/gc-stock.c @@ -305,6 +305,9 @@ STATIC_INLINE void gc_setmark_pool(jl_ptls_t ptls, jl_taggedvalue_t *o, STATIC_INLINE void gc_setmark(jl_ptls_t ptls, jl_taggedvalue_t *o, uint8_t mark_mode, size_t sz) JL_NOTSAFEPOINT { + if (o->bits.in_image) { + return; + } if (sz <= GC_MAX_SZCLASS) { gc_setmark_pool(ptls, o, mark_mode); } From 5965407670d0f0c4053cfd1bbd4aa7ddd0b28246 Mon Sep 17 00:00:00 2001 From: Gabriel Baraldi Date: Mon, 23 Sep 2024 16:59:45 -0300 Subject: [PATCH 5/6] Revert back to globally rooting the expr global since it's simpler and does work --- src/codegen.cpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/codegen.cpp b/src/codegen.cpp index 0811826ea6f4d..a7d24b1b8340b 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -6378,13 +6378,9 @@ static jl_cgval_t emit_expr(jl_codectx_t &ctx, jl_value_t *expr, ssize_t ssaidx_ jl_value_t *val = expr; if (jl_is_quotenode(expr)) val = jl_fieldref_noalloc(expr, 0); - if (jl_is_method(ctx.linfo->def.method)) // toplevel exprs are already rooted - val = jl_ensure_rooted(ctx, val); - else { - jl_task_t *ct = jl_current_task; - jl_gc_force_mark_old(ct->ptls, val); // Make the value old and marked + put it in the remset - } // Top level expressions are no longer rooted after execution - // so storing a new object into an old binding will cause GC corruption + // Toplevel exprs are rooted but because codegen assumes this is constant, it removes the write barriers for this code. + // This means we have to globally root the value here. (The other option would be to change how we optimize toplevel code) + val = jl_ensure_rooted(ctx, val); return mark_julia_const(ctx, val); } From 49df6e9ee171cb4ad0aabe6af69e6dbcdbef93f5 Mon Sep 17 00:00:00 2001 From: Gabriel Baraldi Date: Tue, 24 Sep 2024 11:21:17 -0300 Subject: [PATCH 6/6] Revert changes taht are no longer used in the PR --- src/gc-stock.c | 33 --------------------------------- src/jl_exported_funcs.inc | 1 - src/julia_internal.h | 1 - 3 files changed, 35 deletions(-) diff --git a/src/gc-stock.c b/src/gc-stock.c index 919219eb31f5d..d25f8917f302d 100644 --- a/src/gc-stock.c +++ b/src/gc-stock.c @@ -305,9 +305,6 @@ STATIC_INLINE void gc_setmark_pool(jl_ptls_t ptls, jl_taggedvalue_t *o, STATIC_INLINE void gc_setmark(jl_ptls_t ptls, jl_taggedvalue_t *o, uint8_t mark_mode, size_t sz) JL_NOTSAFEPOINT { - if (o->bits.in_image) { - return; - } if (sz <= GC_MAX_SZCLASS) { gc_setmark_pool(ptls, o, mark_mode); } @@ -4016,36 +4013,6 @@ JL_DLLEXPORT void jl_gc_schedule_foreign_sweepfunc(jl_ptls_t ptls, jl_value_t *o arraylist_push(&ptls->gc_tls.sweep_objs, obj); } -void jl_gc_force_mark_old(jl_ptls_t ptls, jl_value_t *v) JL_NOTSAFEPOINT -{ - jl_taggedvalue_t *o = jl_astaggedvalue(v); - jl_datatype_t *dt = (jl_datatype_t*)jl_typeof(v); - size_t dtsz = jl_datatype_size(dt); - if (o->bits.gc == GC_OLD_MARKED) - return; - o->bits.gc = GC_OLD_MARKED; - if (dt == jl_simplevector_type) { - size_t l = jl_svec_len(v); - dtsz = l * sizeof(void*) + sizeof(jl_svec_t); - } - else if (dt->name == jl_genericmemory_typename) { - dtsz = sizeof(jl_genericmemory_t); - } - else if (dt == jl_module_type) { - dtsz = sizeof(jl_module_t); - } - else if (dt == jl_task_type) { - dtsz = sizeof(jl_task_t); - } - else if (dt == jl_symbol_type) { - return; - } - gc_setmark(ptls, o, GC_OLD_MARKED, dtsz); - if (dt->layout->npointers != 0) - jl_gc_queue_root(v); -} - - #ifdef __cplusplus } #endif diff --git a/src/jl_exported_funcs.inc b/src/jl_exported_funcs.inc index aa12725282034..7abf2b055bb8c 100644 --- a/src/jl_exported_funcs.inc +++ b/src/jl_exported_funcs.inc @@ -170,7 +170,6 @@ XX(jl_gc_small_alloc) \ XX(jl_gc_queue_multiroot) \ XX(jl_gc_queue_root) \ - XX(jl_gc_force_mark_old) \ XX(jl_gc_safepoint) \ XX(jl_gc_schedule_foreign_sweepfunc) \ XX(jl_gc_set_cb_notify_external_alloc) \ diff --git a/src/julia_internal.h b/src/julia_internal.h index fc3d79c3047ab..f00667d016796 100644 --- a/src/julia_internal.h +++ b/src/julia_internal.h @@ -374,7 +374,6 @@ jl_value_t *jl_gc_small_alloc_noinline(jl_ptls_t ptls, int offset, int osize); jl_value_t *jl_gc_big_alloc_noinline(jl_ptls_t ptls, size_t allocsz); JL_DLLEXPORT int jl_gc_classify_pools(size_t sz, int *osize) JL_NOTSAFEPOINT; -JL_DLLEXPORT void jl_gc_force_mark_old(jl_ptls_t ptls, jl_value_t *v); void gc_sweep_sysimg(void);