Update to MMTk core PR #1457

[qinsoon]

No description provided.

[qinsoon]

The segfault is from the following call stack

rb_current_execution_context (vm_core.h:2166)   <---- segfault here, as ec is null
rb_thread_check_ints (thread.c:1469)
rb_gc_impl_gc_disable (gc/default/default.c:1661)
gc_disable_no_rest / rb_gc_disable_no_rest (gc.c)
wmap_compact (weakmap.c:113)
rb_gc_update_object_references (gc.c:4643)
rb_mmtk_handle_weak_references (mmtk_support.c:996)

But a bigger problem is that the thread is doing GC, and is trying to disable collection while in a GC -- mmtk/mmtk-core#1457 forbits that.

[wks]

Here is what happened.

In CRuby, xmalloc is an accounted form of malloc. If it allocated a given amount of memory, it may trigger GC. But it may only trigger GC if GC is enabled.

In the failed test, the error happens during a copying GC when using MMTk. It was iterating over a global hash table (allocated by xmalloc, not in GC heap) and updating the keys and values. Because the keys are object references, and CRuby is not using address-based hashing, if a key is moved, it needs to be removed from the hash table and inserted again. By inserting into the hash table it may result in rehashing, and calling xmalloc to re-allocate the hash table payload.

We know that xmalloc may trigger GC, but since we are doing GC, we can't trigger GC. So CRuby's existing code does this:

1. Disable GC (and record if GC was already disabled before)
2. Insert key-value entry to the hash table, which may try to trigger GC but will not because GC is disabled.
3. Enable GC if it was step 1 that disabled the GC.

It worked in CRuby's default GC because disabling GC simply sets a flag.

While using MMTk, we implemented rb_gc_impl_gc_disable in a loop: if a mutator failed to disable GC, it means GC is already triggered, and it will block until the next GC. It called rb_thread_check_ints which assumes the "execution context" (EC) is not NULL. It is not null for mutator threads, but null for GC threads. Segmentation fault happened for this reason.

This should be straightforward to fix.

1. When using MMTk, it doesn't make sense to disable and enable GC around xmalloc during GC. We can simply skip them.
2. rb_thread_check_ints checks for "interrupts" which is like throwing exceptions between threads, similar to Java's Thread.interrupt(). We should use a different mechanism to block the current thread for GC, similar to the one used for allocation.

[qinsoon]

Thanks for the explanation. I thought rb_thread_check_ints is the mechanism for a thread to stop in a safepoint for a GC, is it not?

[wks]

I made a few changes.

It now calls rb_mmtk_block_for_gc to block the mutator if it fails to disable GC.

DURING_GC_COULD_MALLOC_REGION_START and DURING_GC_COULD_MALLOC_REGION_END are made no-op when using MMTk.

I also disabled two test cases about GC.enable and GC.disable. MMTk-core now implements nesting count, so if GC is disabled twice, it needs two mmtk_enable_collection to re-enable GC. But CRuby does not implement this. For CRuby, the disabled-ness of GC is a single flag that can be set or cleared by GC.disable and GC.enable, and there is no nest count. I could implement this on the Ruby side, but I hesitate to implement it because it is obviously the "wrong" behavior. Even with GC.disable and GC.enable returning the old GC state as return values, it still won't work properly if two threads interleave when try to disable-enable GC. For example,

• Initially GC was enabled.
• T1: was_disabled1 = GC.disable (Sees false)
• T2: was_disabled2 = GC.disable (Sees true)
• T1: GC.enable if !was_disabled1 (because was_disabled1 == false, it enables the GC)
• Now T2 has not left the region between GC.disable and GC.enable, but GC has already been re-enabled by T1.
• T2: GC.enable if !was_disabled2 (Doesn't matter. GC has already been enabled.)

So I don't think it makes sense to mimic the behavior of CRuby's default GC. Instead, CRuby's default GC should be fixed.

[wks]

There are many test cases that depend on CRuby's existing non-nest-counted behavior.

There are utilities like EnvUtils.without_gc that makes sure there is no GC in a region, and restores to the state before it.

  def without_gc
    prev_disabled = GC.disable
    yield
  ensure
    GC.enable unless prev_disabled
  end

It is used like this:

  def test_match_no_match_no_matchdata
    EnvUtil.without_gc do
      h = {}
      ObjectSpace.count_objects(h)
      prev_matches = h[:T_MATCH] || 0
      _md = /[A-Z]/.match('1') # no match
      ObjectSpace.count_objects(h)
      new_matches = h[:T_MATCH] || 0
      assert_equal prev_matches, new_matches, "Bug [#20104]"
    end
  end

And there is GCDisabledChecker which checks after each test case that the GC-enabled state is the same as before the test case.

This means if we change the behavior of GC.enable and GC.disable to MMTk's counted behavior, many test cases and test utilities need to be rewritten or disabled.

But implementing CRuby's old non-nesting behavior is also difficult. If one thread triggered the GC and multiple threads want to disable GC, then all of them need to block for GC. But if the GC-enabled state is non-nesting, only one thread can change the GC-enabled state, and this means it needs to be protected by a mutex. We can't just let CRuby threads block on a Mutex instead of a safepoint.

[qinsoon]

Would it be helpful if we allow a build-time flag (or a runtime flag) to make the nesting depth to be at most 1? What else do we need to support Ruby disable semantics?

[wks]

Would it be helpful if we allow a build-time flag (or a runtime flag) to make the nesting depth to be at most 1? What else do we need to support Ruby disable semantics?

Yes. I think it will make things easier if mmtk-core can provide the following two functions:

• MMTK::disable_collection(level: Option<usize>) -> Result<bool, GcStatus>
• MMTK::enable_collection(level: Option<usize>) -> bool

The two functions will increase or decrease the disabled depth by value if level == Some(value). But if level == None, then disable_collection will increase the depth from 0 to 1, but will not increase it further if the depth is already positive; and enable_collection will directly reduce the depth to 0 and enable GC.

Actually, providing either one of them is sufficient to support the use case of CRuby, but for consistency, I think it is helpful to provide them both.

Another reason for introducing the level argument is cleaning up. There could be a case where a mutator thread crashes (e.g. by throwing exceptions). Some clean-up routime can neutralize the nested disable_collection calls with one invocation of enable_collection.

[wks]

The test case TestProcess#test_clock_gettime_unit failed because it expects the invocations of Time.now and Process.clock_gettime to be within one second. But either the GitHub runner is busy or a GC is triggered in between and they ended up being 1.6749534606933594 seconds apart.

I'll rerun the test.