Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Values may cache twice across JIT boundary #6164

Open
headius opened this issue Apr 2, 2020 · 0 comments
Open

Values may cache twice across JIT boundary #6164

headius opened this issue Apr 2, 2020 · 0 comments

Comments

@headius
Copy link
Member

headius commented Apr 2, 2020

Several language constructs will cache permanently or semi-permanently after the first execution. Constants will look up once and use the cached value unless invalidated. Immediates will be retrieved or constructed once and cached permanently.

However when we transition from one tier to another, like simple interpreter to full interpreter or interpreter to JIT, we lose those cached values and may execute that code again.

For most constructs this is just wasted work; literals will be constructed again and block bodies will be instantiated again. However constants often have side effect behavior that fires on lookup, like warnings.

That latter situation leads to this failure running CRuby tests with interpreter and JIT threshold set to zero (immediately JIT).

[1/1] TestModule#test_deprecate_nil_constant"/Users/headius/projects/jruby/test/mri/ruby/test_module.rb:1533: warning: constant TestModule::FALSE is deprecated\n"
"/Users/headius/projects/jruby/test/mri/ruby/test_module.rb:1533: warning: constant TestModule::FALSE is deprecated\n"
 = 0.02 s
  1) Failure:
TestModule#test_deprecate_nil_constant [/Users/headius/projects/jruby/test/mri/ruby/test_module.rb:1534]:
/Users/headius/projects/jruby/test/mri/ruby/test_module.rb:1533: warning: constant TestModule::FALSE is deprecated
/Users/headius/projects/jruby/test/mri/ruby/test_module.rb:1533: warning: constant TestModule::FALSE is deprecated
.
<1> expected but was
<2>.

The test has this code, which expects the FALSE and NIL constants to be looked up exactly once:

  NIL = nil
  FALSE = false
  deprecate_constant(:NIL, :FALSE)

  def test_deprecate_nil_constant
    w = EnvUtil.verbose_warning {2.times {FALSE}}
    assert_equal(1, w.scan("::FALSE").size, w)
    w = EnvUtil.verbose_warning {2.times {NIL}}
    assert_equal(1, w.scan("::NIL").size, w)
  end

This happens because even with jit.threshold set to zero, we don't immediately branch to the optimized version of the IR. The trigger for the jit happens too far down the simple-interpreter call paths in InterpretedIRBlockBody; by the time we've triggered it, we're already committed to the simple interpreter. So it runs once, and then the second time we use the new IR and cache the value again.

There's various partial fixes possible, but most of them will only apply to the simple-to-full transition. The interpreter-to-jit transition is much harder to fix, since the JIT uses entirely different mechanisms for caching data across calls (specifically, static fields on jitted classes or indy call sites).

  • Make sure instructions and operands that cache data at runtime pass that cached data on to the cloned versions in the new IR. This may have unintended side effects, however.
  • Move the block jit threshold check and JIT request earlier in the call path, so we aren't already committed to the simple interpreter path at that point.
  • Switch up all caching to use storage associated with the runtime rather than the IR or jitted code structures. Traversing the runtime to get these values will be less efficient, however, and will never constant fold in the JIT.

For now we will tag the above issue, since double-warning is a very minor side effect, and unlikely to affect any real code. Note that constant lookups resulting in const_missing never cache any results.

headius added a commit to headius/jruby that referenced this issue Jan 12, 2023
Since jruby#6164 was filed, we started setting jit.background=false
whenever jit.threshold=0, since the intent is that all methods
JIT before executing. This prevents us failing the deprecated
constant test because we no longer cache once in interpreter and
again in JIT. It does not fix the issue that when transitiong to
JIT code we will recache constants and other values so jruby#6164 is
still an issue (if it is really an issue).
edipofederle pushed a commit to edipofederle/jruby that referenced this issue Feb 8, 2023
Since jruby#6164 was filed, we started setting jit.background=false
whenever jit.threshold=0, since the intent is that all methods
JIT before executing. This prevents us failing the deprecated
constant test because we no longer cache once in interpreter and
again in JIT. It does not fix the issue that when transitiong to
JIT code we will recache constants and other values so jruby#6164 is
still an issue (if it is really an issue).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant