Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v8]
On Tue, 7 Dec 2021 10:42:50 GMT, Roman Kennke wrote: >> 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: >> - [x] tier1 >> - [x] tier2 >> - [x] tier3 >> - [ ] tier4 > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Add improved test by @plevart 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. - PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v8]
On Fri, 10 Dec 2021 14:44:07 GMT, Kevin Rushforth wrote:
> 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](https://openjdk.java.net/jeps/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
-
PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v8]
On Tue, 7 Dec 2021 10:42:50 GMT, Roman Kennke wrote: >> 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: >> - [x] tier1 >> - [x] tier2 >> - [x] tier3 >> - [ ] tier4 > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Add improved test by @plevart 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](https://openjdk.java.net/jeps/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. - PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v8]
On Tue, 7 Dec 2021 10:42:50 GMT, Roman Kennke wrote: >> 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: >> - [x] tier1 >> - [x] tier2 >> - [x] tier3 >> - [ ] tier4 > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Add improved test by @plevart 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? - PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v8]
> 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: > - [x] tier1 > - [x] tier2 > - [x] tier3 > - [ ] tier4 Roman Kennke has updated the pull request incrementally with one additional commit since the last revision: Add improved test by @plevart - Changes: - all: https://git.openjdk.java.net/jdk/pull/6375/files - new: https://git.openjdk.java.net/jdk/pull/6375/files/e6e34cea..cfaf08cd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6375=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6375=06-07 Stats: 102 lines in 2 files changed: 83 ins; 18 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6375.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6375/head:pull/6375 PR: https://git.openjdk.java.net/jdk/pull/6375
