Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v4]
On Thu, 2 Dec 2021 14:24:06 GMT, Roman Kennke wrote:
>> src/java.base/share/classes/java/io/ObjectStreamClass.java line 2133:
>>
>>> 2131: if (oldReflector != null) {
>>> 2132: reflector = oldReflector;
>>> 2133: }
>>
>> Map.computeIfAbsent(key, () -> new FieldReflector(matchFields, localDesc));
>> might be more compact.
>
> 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.
Yes, I did computeIfAbsent() originally just to find out handling check
exception/wrapping/unwrapping would make the code much more complex.
-
PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v4]
On Wed, 1 Dec 2021 21:09:14 GMT, Roger Riggs wrote:
> 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).
> src/java.base/share/classes/java/io/ObjectStreamClass.java line 2133:
>
>> 2131: if (oldReflector != null) {
>> 2132: reflector = oldReflector;
>> 2133: }
>
> Map.computeIfAbsent(key, () -> new FieldReflector(matchFields, localDesc));
> might be more compact.
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.
> test/jdk/java/io/ObjectStreamClass/TestOSCClassLoaderLeak.java line 52:
>
>> 50: Class loadClass =
>> myOwnClassLoader.loadClass("ObjectStreamClass_MemoryLeakExample");
>> 51: Constructor con = loadClass.getConstructor();
>> 52: con.setAccessible(true);
>
> Isn't the constructor already public?
>
Yes, but:
test TestOSCClassLoaderLeak.run(): failure
java.lang.IllegalAccessException: class TestOSCClassLoaderLeak cannot access a
member of class ObjectStreamClass_MemoryLeakExample with modifiers "public"
-
PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v4]
On Wed, 1 Dec 2021 12:18:05 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:
>
> Remove unused EntryFuture inner class from ObjectSteamClass
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.
src/java.base/share/classes/java/io/ObjectStreamClass.java line 2133:
> 2131: if (oldReflector != null) {
> 2132: reflector = oldReflector;
> 2133: }
Map.computeIfAbsent(key, () -> new FieldReflector(matchFields, localDesc));
might be more compact.
test/jdk/java/io/ObjectStreamClass/TestOSCClassLoaderLeak.java line 52:
> 50: Class loadClass =
> myOwnClassLoader.loadClass("ObjectStreamClass_MemoryLeakExample");
> 51: Constructor con = loadClass.getConstructor();
> 52: con.setAccessible(true);
Isn't the constructor already public?
-
PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v4]
> 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: Remove unused EntryFuture inner class from ObjectSteamClass - Changes: - all: https://git.openjdk.java.net/jdk/pull/6375/files - new: https://git.openjdk.java.net/jdk/pull/6375/files/2ed745b3..b7d53adc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6375=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6375=02-03 Stats: 65 lines in 1 file changed: 0 ins; 65 del; 0 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
