RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v7]

[Peter Levart]

On Mon, 6 Dec 2021 12:12:44 GMT, Roman Kennke <rkennke at 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