Use a normal ClassValue for all such cases#8844
Conversation
|
Additional tweak to synchronize the class value calculators didn't seem to solve the related test failures, so there's a bit more work required here. And of course guarding the new logic behind an option. |
|
Upped the brutality level of my repro script to include spinning up and tearing down JRuby instances. It's blazing through GC cycles and using most of max heap but it levels off nicely for both heap usage (red) and metaspace (pink). Script: loop do
runtime = org.jruby.Ruby.new_instance
runtime.eval_scriptlet <<~RUBY
class Foo
def accept(x)
end
end
ary = java.util.ArrayList.new([1])
i = 0
1000.times {
i += 1
ary.for_each(Foo.new)
}
RUBY
runtime.tear_down
print '.'
endGraph:
|
|
I realized a problem with the new torture test: because the ClassValue objects rooting these classes go away with the JRuby instance, they don't leak in any of the modes. It does show, however, that classes are not leaking across instances in other the old mode or the new mode. Using the original single-runtime test, I can confirm that the old mode leaks very rapidly and chokes, while the new mode levels off and continues running just fine. Old (shorter because it started to hit metaspace limit): New: I will push an update shortly that maintains compatibility with the JRuby 9.4 ClassValue implementation and provides a new option for configuring the fixed mode. This mode will be the default in JRuby 10.0.1. |
|
From the most recent commit, which should align this work with JRuby 9.4 behavior and allow configuring the new STABLE option to fix #8842: |
|
Tests appear to be green other than an oddity in sequel. The functional parts of this PR can be reviewed now. I still need to figure out some tests. |
JRuby includes two versions of ClassValue: one with a hard- referenced Map from Class to value, and one based on a normal ClassValue but with the added oddity that it hard-references its values while storing them in the ClassValue slot weakly. This was originally intended to allow the values to collect properly when the ClassValue collected, but on recent testing it seems to just leak in a different way. This change siwtches everything to normal ClassValue. Given this change, the example cases in jruby#8842 no longer leak, nor does a more aggressive torture test that also spins up and tears down JRuby runtimes as well as interface implementations. A 30-minute run of that test uses a lot of memory and GCs heavily, but does level off in both heap and metaspace. Forced GCs reduce both nearly back to startup levels.
MapBasedClassValue added synchronization to its computeValue call in 44895a5 in an attempt to align with ClassValue's requirement that values only be calculated once. This would be better as a lock-free atomic operation, but I add back the synch wrappers for now to maintain compatibility.
In order to adopt the JDK ClassValue for our own ClassValue API, and maintain compatibility with JRuby 9.4, this patch includes the following changes: * Add a StableClassValue that uses Java's ClassValue but with a once-only computation guarantee. This is the mode that fixes jruby#8842. * Deprecate the old invokedynamic.class.values option, but allow it to still select the Java7ClassValue implementation. * Add a new ji.class.values option based on ClassValue.Type enum and the following values: * STABLE, the stable java.lang.ClassValue implementation * HARD_MAP, the default hard-referenced Map implementation * HARD_VALUES, the Java7 version JRuby 9.4 will continue to default to the HARD_MAP version, to avoid introducing the new behavior where it is not expected. JRuby 10 will merge this change and default to STABLE starting with 10.0.1. See discussion of ClassValue and stability on the mlvm-dev mailing list here: https://mail.openjdk.org/pipermail/mlvm-dev/2025-May/006933.html
headius
force-pushed
the
weaker_class_values
branch
from
5fea68e to
2483a8e
Compare
|
The sequel failure was due to a new MariaDB test mode being run, so I've merged in the appropriate configs and repushed. |
|
A 12h graph of heap and metaspace stability with stable class values:
|
This test works by using OpenJDK flags for tracing loading and unloading of classes. An InterfaceImpl class is generated between output of "started" and "exiting" and the class tracing output is tested to ensure expected loads and unloads. The new STABLE mode for JI class values should include output showing that the InterfaceImpl gets loaded and unloaded. The older HARD_MAP mode should show loading only. See jruby#8842 and PR jruby#8844
|
I have pushed a regression test that confirms both modes' behaviors:
@kares @enebo check my work here. This took too long to figure out a test for, and might be fragile, but it's very difficult to detect class unloading on JVM. |
After some discussion we decided that eliminating the leak that was introduced in 9.4.0.0 was more important than maintaining the leaky behavior, even though only one user has reported an issue. The risk was deemed minimal; the only behavioral change is that the hidden, transient InterfaceImpl classes attached to duck-typed objects will now eventually be unloaded and collected. Since these classes were being generated fresh for each new object, there's no impact to reuse or reloading. If instance of these classes are memoized by some Java code, they will continue to exist as before. Users that have concerns about running with the new "stable" JI class values can revert to the old behavior by passing a flag to JRuby (or setting the equivalent JVM property): -Xji.class.values=HARD_MAP The new behavior is more stable and no longer leaks classes, and we believe it is the better experience for JRuby users. See jruby#8842 and jruby#8844
2 participants
JRuby includes two versions of ClassValue: one with a hard- referenced Map from Class to value, and one based on a normal ClassValue but with the added oddity that it hard-references its values while storing them in the ClassValue slot weakly. This was originally intended to allow the values to collect properly when the ClassValue collected, but on recent testing it seems to just leak in a different way.
This change siwtches everything to normal ClassValue. Given this change, the example cases in #8842 no longer leak, nor does a more aggressive torture test that also spins up and tears down JRuby runtimes as well as interface implementations. A 30-minute run of that test uses a lot of memory and GCs heavily, but does level off in both heap and metaspace. Forced GCs reduce both nearly back to startup levels.
Note this is a work in progress; we will likely not want to change the default behavior for 9.4.x, so this will need to be guarded behind an option relating to the existing invokedynamic.class.values toggle. It may be safe enough as-is for JRuby 10 as that is still stabilizing.