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

8277072: ObjectStreamClass caches keep ClassLoaders alive #6375

Closed
wants to merge 11 commits into from

Conversation

rkennke
Copy link
Contributor

@rkennke rkennke commented Nov 12, 2021

The caches in ObjectStreamClass basically map WeakReference to SoftReference, 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:

  • tier1
  • tier2
  • tier3
  • tier4

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8277072: ObjectStreamClass caches keep ClassLoaders alive

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6375/head:pull/6375
$ git checkout pull/6375

Update a local copy of the PR:
$ git checkout pull/6375
$ git pull https://git.openjdk.java.net/jdk pull/6375/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6375

View PR using the GUI difftool:
$ git pr show -t 6375

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6375.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 12, 2021

👋 Welcome back rkennke! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 12, 2021

@rkennke The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the core-libs label Nov 12, 2021
@rkennke rkennke marked this pull request as draft Nov 15, 2021
@rkennke rkennke marked this pull request as ready for review Nov 15, 2021
@openjdk openjdk bot added the rfr label Nov 15, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 15, 2021

@jddarcy
Copy link
Member

@jddarcy jddarcy commented Nov 15, 2021

If the intent of this change is to alter the lifetimes of the objects in question in a meaningful way, I recommend a CSR for the behavioral compatibility impact.

@rkennke
Copy link
Contributor Author

@rkennke rkennke commented Nov 15, 2021

If the intent of this change is to alter the lifetimes of the objects in question in a meaningful way, I recommend a CSR for the behavioral compatibility impact.

It would be hard for application code to observe this change: before the change, a ClassLoader and its classes could be lingering in the cache longer than necessary, even if otherwise not reachable. With the change, they would be reclaimed as soon as they become unreachable. This could only be observed, if application code holds onto ClassLoader or Class instances via Weak or PhantomReference, and even then I am not sure if that qualifies as 'meaningful'.

@rkennke
Copy link
Contributor Author

@rkennke rkennke commented Nov 29, 2021

Ping? Can I please get another review? Thanks!

@plevart
Copy link
Contributor

@plevart plevart commented Nov 29, 2021

If the intent of this change is to alter the lifetimes of the objects in question in a meaningful way, I recommend a CSR for the behavioral compatibility impact.

It would be hard for application code to observe this change: before the change, a ClassLoader and its classes could be lingering in the cache longer than necessary, even if otherwise not reachable. With the change, they would be reclaimed as soon as they become unreachable. This could only be observed, if application code holds onto ClassLoader or Class instances via Weak or PhantomReference, and even then I am not sure if that qualifies as 'meaningful'.

Hi Roman,

What your patch changes is the following:

ConcurrentHashMap<WeakReference<Class<?>>, SoftReference<ObjectStreamClass>>

into:

ConcurrentHashMap<WeakReference<Class<?>>, WeakReference<ObjectStreamClass>>

While it is true that when the Class object used in a weak key is not reachable any more by the app, it is not sensible to hold on to the value any longer so in that respect SoftReference is to "storng" of a weakness. But while the Class object is still reachable by the app, the app expects to obtain the ObjectStreamClass (the value) from the cache at least most of the time. If you change the SoftReference into WeakReference, the ObjectStreamClass might get GC-ed even while in the middle of stream deserialization.

ObjectStream class pre-dates java.lang.invoke (MethodHandles), so it uses its own implementation of weak caching. But since MethodHandles, there is a class called ClassValue that would solve these problem with more elegance, because it ties the lifetime of the value (ObjectStreamClass) to the lifetime of the Class key (Class object has a strong reference to the associated value) while the Class key is only Weakly referenced.

@rkennke
Copy link
Contributor Author

@rkennke rkennke commented Nov 29, 2021

While it is true that when the Class object used in a weak key is not reachable any more by the app, it is not sensible to hold on to the value any longer so in that respect SoftReference is to "storng" of a weakness. But while the Class object is still reachable by the app, the app expects to obtain the ObjectStreamClass (the value) from the cache at least most of the time. If you change the SoftReference into WeakReference, the ObjectStreamClass might get GC-ed even while in the middle of stream deserialization.

I don't quite understand this: If the Class object is still reachable by the app, 1. a weak reference would not get cleared and 2. the Class's ClassLoader would not get unloaded. Conversely, if it's not reachable by the app anymore, then the key in the cache would get cleared, and we would not find the ObjectStreamClass anyway. Except that the OSC holds onto the Class object by a SoftReference, so it would effectively prevent getting cleared (and get unloaded).

ObjectStream class pre-dates java.lang.invoke (MethodHandles), so it uses its own implementation of weak caching. But since MethodHandles, there is a class called ClassValue that would solve these problem with more elegance, because it ties the lifetime of the value (ObjectStreamClass) to the lifetime of the Class key (Class object has a strong reference to the associated value) while the Class key is only Weakly referenced.

Hmm, sounds nice. Do you think that would work in the context of OSC?

@plevart
Copy link
Contributor

@plevart plevart commented Nov 29, 2021

A patch is worth a thousand words. Here's what I meant when I said this could be elegantly solved with ClassValue:

plevart@6e16e5e

Note this is not tested. Just an idea.

@plevart
Copy link
Contributor

@plevart plevart commented Nov 29, 2021

I don't quite understand this: If the Class object is still reachable by the app,

  1. a weak reference would not get cleared and
  2. the Class's ClassLoader would not get unloaded.

...but the ObjectStreamClass instance could still get GC-ed, because it is held in the map using WeakReference. The fact that associated Class is still reachable does not mean that the ObjectStreamClass instance is!

@openjdk
Copy link

@openjdk openjdk bot commented Nov 30, 2021

⚠️ @rkennke This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

@rkennke
Copy link
Contributor Author

@rkennke rkennke commented Nov 30, 2021

A patch is worth a thousand words. Here's what I meant when I said this could be elegantly solved with ClassValue:

plevart@6e16e5e

Note this is not tested. Just an idea.

Very nice!
I've merged your change, it passes the testcase, and I've also run tier1-3 successfully. Do you want to take over? It's mostly your change now, anyway (except for the testcase). Or do you want me to finish it?

@plevart
Copy link
Contributor

@plevart plevart commented Dec 1, 2021

I think most "hard work" (the tests) is still yours. I just removed a chunk of legacy code and replaced it with one-liners :-). I'm glad that this actually works! Please, continue...

@plevart
Copy link
Contributor

@plevart plevart commented Dec 1, 2021

...I think that you could remove now obsolete java.io.ObjectStreamClass.EntryFuture nested class. It's not used any more.

It would be nice to follow-up this patch with patches that make use of ClassValue also for:

  • java.io.ObjectInputStream.Caches#subclassAudits
  • java.io.ObjectOutputStream.Caches#subclassAudits

...this way the common static machinery like:

  • java.io.ObjectStreamClass#processQueue
  • java.io.ObjectStreamClass.WeakClassKey
    ...could get removed as it is not used in ObjectStreamClass any more.

@rkennke
Copy link
Contributor Author

@rkennke rkennke commented Dec 1, 2021

...I think that you could remove now obsolete java.io.ObjectStreamClass.EntryFuture nested class. It's not used any more.

Done.

It would be nice to follow-up this patch with patches that make use of ClassValue also for:

* java.io.ObjectInputStream.Caches#subclassAudits

* java.io.ObjectOutputStream.Caches#subclassAudits

...this way the common static machinery like:

* java.io.ObjectStreamClass#processQueue

* java.io.ObjectStreamClass.WeakClassKey
  ...could get removed as it is not used in ObjectStreamClass any more.

I filed: https://bugs.openjdk.java.net/browse/JDK-8278065
And here comes the PR: #6637

Thanks!

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Without the use of SoftReference, memory pressure won't release any of the cached info.
That seems to swing the other way from overly aggressively freeing memory with the WeakReference (and needing to recompute) as the change originally proposed.
Its hard to tell in what environments it might be observed.

reflector = new FieldReflector(matchFields(fields, localDesc));
var oldReflector = clReflectors.putIfAbsent(key, reflector);
if (oldReflector != null) {
reflector = oldReflector;
}
Copy link
Contributor

@RogerRiggs RogerRiggs Dec 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map.computeIfAbsent(key, () -> new FieldReflector(matchFields, localDesc));
might be more compact.

Copy link
Contributor Author

@rkennke rkennke Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be nicer, indeed. Problem is that matchFields throws an InvalidClassException, and that would have to get passed through the lambda.
Also, that problem is pre-existing and not related to the change.

Copy link
Contributor

@plevart plevart Dec 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I did computeIfAbsent() originally just to find out handling check exception/wrapping/unwrapping would make the code much more complex.

TestClassLoader myOwnClassLoader = new TestClassLoader();
Class<?> loadClass = myOwnClassLoader.loadClass("ObjectStreamClass_MemoryLeakExample");
Constructor con = loadClass.getConstructor();
con.setAccessible(true);
Copy link
Contributor

@RogerRiggs RogerRiggs Dec 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the constructor already public?

Copy link
Contributor Author

@rkennke rkennke Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but:

test TestOSCClassLoaderLeak.run(): failure
java.lang.IllegalAccessException: class TestOSCClassLoaderLeak cannot access a member of class ObjectStreamClass_MemoryLeakExample with modifiers "public"

@rkennke
Copy link
Contributor Author

@rkennke rkennke commented Dec 2, 2021

Without the use of SoftReference, memory pressure won't release any of the cached info. That seems to swing the other way from overly aggressively freeing memory with the WeakReference (and needing to recompute) as the change originally proposed. Its hard to tell in what environments it might be observed.

Right. The problem with the original code was that the softreference would keep the class from getting unloaded, except when under pressure. Now that the cached value is tied to the object lifetime using ClassValue, we can relatively easily use SoftReference to also make it sensitive to memory pressure. I factored this code out into its own class to avoid making a mess, and to be able to reuse it in subclassAudits (see #6637).

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

ObjectStreamClass may have an unnecesary import of SoftReference.

Otherwise, looks good to me.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 2, 2021

@rkennke This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8277072: ObjectStreamClass caches keep ClassLoaders alive

Reviewed-by: rriggs, plevart

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 180 new commits pushed to the master branch:

  • ba86dd4: 8278445: ProblemList tools/jpackage/share/IconTest.java on macosx-x64
  • 92aa75b: 8274903: Zero: Support AsyncGetCallTrace
  • 8345453: 8272392: Lanai: SwingSet2. Black background on expanding tree node
  • 9b74749: 8276660: Scalability bottleneck in java.security.Provider.getService()
  • 2478158: 8277361: java/nio/channels/Channels/ReadXBytes.java fails with OOM error
  • 8af3b27: 8277850: C2: optimize mask checks in counted loops
  • 3e93e0b: 8276769: -Xshare:auto should tolerate problems in the CDS archive
  • 79165b7: 8278324: Update the --generate-cds-archive jlink plugin usage message
  • 40d726b: 8278310: Improve logging in CDS DynamicLoaderConstraintsTest.java
  • e4852c6: 8277998: runtime/cds/appcds/loaderConstraints/DynamicLoaderConstraintsTest.java#custom-cl-zgc failed "assert(ZAddress::is_marked(addr)) failed: Should be marked"
  • ... and 170 more: https://git.openjdk.java.net/jdk/compare/ad51d0692534744d04a32959e7e50ee5e87adff5...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Dec 2, 2021
@rkennke
Copy link
Contributor Author

@rkennke rkennke commented Dec 2, 2021

Thanks, @RogerRiggs!
@plevart does that also look reasonable to you?

reflector = new FieldReflector(matchFields(fields, localDesc));
var oldReflector = clReflectors.putIfAbsent(key, reflector);
if (oldReflector != null) {
reflector = oldReflector;
}
Copy link
Contributor

@plevart plevart Dec 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I did computeIfAbsent() originally just to find out handling check exception/wrapping/unwrapping would make the code much more complex.

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Thanks for the updates. LGTM

}

assertNotNull(ObjectStreamClass.lookup(TestClass.class).getFields());
}
Copy link
Contributor

@plevart plevart Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
}

Copy link
Contributor Author

@rkennke rkennke Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test was a rather crude (but successful) attempt to demonstrate the ClassCastException. Thanks for providing the better testcase. I verified that it succeeds with this PR, and also demonstrates the ClassCastException if I revert my previous change in ClassCache. I pushed this new test, and removed my old one.

@plevart
Copy link
Contributor

@plevart plevart commented Dec 8, 2021

I think this looks good now. Thanks for following through all the changes...

plevart
plevart approved these changes Dec 8, 2021
@rkennke
Copy link
Contributor Author

@rkennke rkennke commented Dec 10, 2021

This should go to openjdk/jdk18 now, right? Can I simply push it there, or do I need to re-open a PR against jdk18?

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Dec 10, 2021

This is a P4 bug. If the priority is correct, it does not meet the criteria to get it into JDK 18 during RDP1, as indicated in JEP 3.

If this is objectively a P3 bug, and really does need to go into JDK 18, then you will need to close this PR and open a new pull request in the jdk18 repo.

@rkennke
Copy link
Contributor Author

@rkennke rkennke commented Dec 10, 2021

This is a P4 bug. If the priority is correct, it does not meet the criteria to get it into JDK 18 during RDP1, as indicated in JEP 3.

If this is objectively a P3 bug, and really does need to go into JDK 18, then you will need to close this PR and open a new pull request in the jdk18 repo.

Hmm, good question. It is kind-of leaking: current implementation prevents unloading of classes that are referenced from the OSC caches, unless memory pressure is high enough to trigger soft-ref-cleaning. Does it qualify for P3 ("Major loss of function."), or even P2 ("Crashes, loss of data, severe memory leak.")? We have users hitting this problem under different circumstances, I'd say it qualifies for P3. Opinions? See:

https://bugzilla.redhat.com/show_bug.cgi?id=2016930

@RogerRiggs
Copy link
Contributor

@RogerRiggs RogerRiggs commented Dec 10, 2021

This fix hasn't had any bake-time and might have some effects that aren't immediately noticeable.
I'd leave it in 19 for the time being. It could be back ported at a later point in time.

@rkennke
Copy link
Contributor Author

@rkennke rkennke commented Dec 10, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Dec 10, 2021

Going to push as commit 8eb453b.
Since your change was applied there have been 214 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Dec 10, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 10, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 10, 2021

@rkennke Pushed as commit 8eb453b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@rkennke rkennke deleted the JDK-8277072 branch Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs integrated
5 participants