On Mon, 6 Dec 2021 12:12:44 GMT, Roman Kennke <rken...@openjdk.org> wrote:
>> The caches in ObjectStreamClass basically map WeakReference<Class> to
>> SoftReference<ObjectStreamClass>, where the ObjectStreamClass also
>> references the same Class. That means that the cache entry, and thus the
>> class and its class-loader, will not get reclaimed, unless the GC determines
>> that memory pressure is very high.
>>
>> However, this seems bogus, because that unnecessarily keeps ClassLoaders and
>> all its classes alive much longer than necessary: as soon as a ClassLoader
>> (and all its classes) become unreachable, there is no point in retaining the
>> stuff in OSC's caches.
>>
>> The proposed change is to use WeakReference instead of SoftReference for the
>> values in caches.
>>
>> Testing:
>> - [x] tier1
>> - [x] tier2
>> - [x] tier3
>> - [ ] tier4
>
> Roman Kennke has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix cast; add testcase to cover memory pressure
Changes requested by plevart (Reviewer).
test/jdk/java/io/ObjectStreamClass/TestOSCClassLoaderLeak.java line 84:
> 82:
> 83:
> assertNotNull(ObjectStreamClass.lookup(TestClass.class).getFields());
> 84: }
I don't quite get this test. It loads ObjectStreamClass_MemoryLeakExample class
from child class loader, constructs an instance from it and calls .toString()
on an instance. This is just to indicate that the class initializer of that
class did lookup an ObjectStreamClass instance for Test class loaded by the
same child loader. OK so far...
Then there is this loop that tries to exhibit some memory pressure while
constantly looking up OSC for another Test class (this time loaded by parent
class loader) presumably to trigger clearing the SoftReference(s) of both
classes loaded by child ClassLoader.... Is this what the loop was supposed to
do?
And finally there is an assertNotNull that does another lookup for OSC of Test
class loaded by parent class loader, retrive its fields and check that the
returned OSC instance as well as the field array are not null. This will always
succeed regardless of what you do before the assertion.
I don't think you need any custom class loading to verify the correctness of
caching. The following two tests pass on old implementation of OSC. Do they
pass on the new one too?
public class ObjectStreamClassCaching {
@Test
public void testCachingEffectiveness() throws Exception {
var ref = lookupObjectStreamClass(TestClass.class);
System.gc();
Thread.sleep(100L);
// to trigger any ReferenceQueue processing...
lookupObjectStreamClass(AnotherTestClass.class);
Assertions.assertFalse(ref.refersTo(null),
"Cache lost entry although memory was not under
pressure");
}
@Test
public void testCacheReleaseUnderMemoryPressure() throws Exception {
var ref = lookupObjectStreamClass(TestClass.class);
pressMemoryHard(ref);
System.gc();
Thread.sleep(100L);
Assertions.assertTrue(ref.refersTo(null),
"Cache still has entry although memory was
pressed hard");
}
// separate method so that the looked-up ObjectStreamClass is not kept on
stack
private static WeakReference<?> lookupObjectStreamClass(Class<?> cl) {
return new WeakReference<>(ObjectStreamClass.lookup(cl));
}
private static void pressMemoryHard(Reference<?> ref) {
try {
var list = new ArrayList<>();
while (!ref.refersTo(null)) {
list.add(new byte[1024 * 1024 * 64]); // 64 MiB chunks
}
} catch (OutOfMemoryError e) {
// release
}
}
}
class TestClass implements Serializable {
}
class AnotherTestClass implements Serializable {
}
-------------
PR: https://git.openjdk.java.net/jdk/pull/6375
