Project

General

Profile

Actions

Bug #19442

closed

Remove USE_RINCGC flag

Added by eightbitraptor (Matthew Valentine-House) over 1 year ago. Updated 6 months ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-core:112446]

Description

GitHub PR #7317

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 (Matthew Valentine-House) 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 (Matthew Valentine-House) 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.

Actions #6

Updated by eightbitraptor (Matthew Valentine-House) over 1 year ago

  • Description updated (diff)

Updated by eightbitraptor (Matthew Valentine-House) 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:

  1. Rhodes 7.5.1 - This gem vendors the complete source code of Ruby 2.3.4 inside platform/shared/ruby.
  2. 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
  3. ruby_memprofiler_pprof - This uses USE_RINCGC, as a default value to GC_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).
  4. zscan 2.0.9 - This gem committed the public header directories include/ruby/internal and include/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.

Actions #10

Updated by eightbitraptor (Matthew Valentine-House) about 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) 6 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.

Actions

Also available in: Atom PDF

Like0
Like0Like1Like0Like0Like0Like0Like0Like1Like0Like0Like0