Bug #19442
closedRemove USE_RINCGC flag
Description
Ruby doesn't compile when USE_RINCGC is disabled. It's also not being tested in CI. As @nobu (Nobuyoshi Nakada) has pointed out in comments on the PR, fixing it is simple. This was fixed in this commit
I think we should remove USE_RINCGC
entirely and always run with incremental GC enabled, because I don't think this flag is being actively used, and removing it will simplify the code and reduce the cognitive overhead of working with the GC.
We have a precedent for this: USE_RGENGC=0
was removed in this commit almost 3 years ago. USE_RINCGC=0
is in a similar state. It has been broken for almost a year (since this commit), and disabled in CI for more than 2 years.
Updated by nobu (Nobuyoshi Nakada) over 1 year ago
I don't think option 1 and 2 are exclusive.
Updated by nobu (Nobuyoshi Nakada) over 1 year ago
Of course I'm not against the removal.
I just say removing the flag and fixing the bug are different stories.
Updated by peterzhu2118 (Peter Zhu) over 1 year ago
I'm for removing many of the untested code paths in gc.c since they may be buggy and are probably not used (like this one, which doesn't even compile so clearly nobody is using it).
Updated by eightbitraptor (Matt V-H) over 1 year ago
nobu (Nobuyoshi Nakada) wrote in #note-1:
I don't think option 1 and 2 are exclusive.
I'm not sure what you mean by this. Do you mean it could be both fixed and removed?
Updated by eightbitraptor (Matt V-H) over 1 year ago
@nobu (Nobuyoshi Nakada) I see you've just fixed it in this commit 😅
Thanks!
I'd still like to see it removed, but I'll update the ticked description to be more reflective of the current status.
Updated by eightbitraptor (Matt V-H) over 1 year ago
Some more context on this ticket. @peterzhu2118 (Peter Zhu) did a gem code search for USE_RINCGC
and that returned 4 distinct gems:
-
Rhodes 7.5.1 - This gem vendors the complete source code of Ruby 2.3.4 inside
platform/shared/ruby
. -
ruby-compiler 0.1.1 - This gem was last released in 2016 and vendored a complete source code of Ruby 2.4. The git repo has since been renamed (to
ruby-packer
) and has been updated more recently. It still vendors a complete copy of Ruby, but this has been updated to 2.7 -
ruby_memprofiler_pprof - This uses
USE_RINCGC
, as a default value toGC_ENABLE_INCREMENTAL_MARK
, although I'm not sure what for. This gem will require an update to work with Ruby 3.3 if this patch gets merged as is (/cc @kjtsanaktsidis (KJ Tsanaktsidis) who works on this gem). -
zscan 2.0.9 - This gem committed the public header directories
include/ruby/internal
andinclude/ruby/backward
into it's source code in May 2020. They've not been updated since.
So it looks like the only codebase that is at risk of being impacted by this change is ruby_memprofiler_pprof
- all the others are vendoring specific versions of Ruby. So upgrading to future versions for them will be a bigger challenge regardless of this patch.
Updated by kjtsanaktsidis (KJ Tsanaktsidis) over 1 year ago
ruby_memprofiler_pprof - This uses USE_RINCGC, as a default value to GC_ENABLE_INCREMENTAL_MARK, although I'm not sure what for.
Because I copied this out of gc.c
- https://github.com/ruby/ruby/blob/c024cc05efa2b5206c4bb45066d3c8e19af7e0d9/gc.c#L471. I think I needed a copy of the GC structures to implement is_pointer_to_heap
in my gem, and of course the shape of those structures depends on the flags Ruby was compiled with.
In any case 1) I'm working (very slowly!) on a different approach to memory profiling now, 2) I completely expect new Ruby versions to break hacks like this, and 3) AFAIK absolutely nobody, myself included, is using this gem.
Updated by byroot (Jean Boussier) over 1 year ago
Similarly I think we should remove all the RGENGC_WB_PROTECTED_*
flags https://github.com/Shopify/ruby/commit/078d46c47cb0e41165e0112cd9669b3256625d8e.
Updated by eightbitraptor (Matt V-H) over 1 year ago
- Status changed from Open to Closed
Applied in changeset git|b3a271665b6d45fe21e775e1c523a040caa509a9.
[Feature #19442] Remove USE_RINCGC flag
Ruby doesn't compile when this is set to 0. Let's remove it.
Updated by ko1 (Koichi Sasada) 11 months ago
These kind of flags are provided to measure the impact with on/off the features.
We can simulate them with parameters, but we can not avoid overhead of write barriers completely, for example.
Anyway it was gone.