[jdk18] Withdrawn: 8277072: ObjectStreamClass caches keep ClassLoaders alive
On Wed, 9 Mar 2022 13:11:27 GMT, Roman Kennke wrote: > Hi all, > > This pull request contains a backport of commit > [8eb453ba](https://github.com/openjdk/jdk/commit/8eb453baebe377697286f7eb32280ca9f1fd7775) > from the [openjdk/jdk](https://git.openjdk.java.net/jdk) repository. > > The commit being backported was authored by Roman Kennke on 10 Dec 2021 and > was reviewed by Roger Riggs and Peter Levart. > > It fixes a memory leak in ObjectStreamClass under certain conditions. See bug > for details. > > Thanks! This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk18/pull/117
Re: [jdk18] RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive
On Wed, 9 Mar 2022 13:11:27 GMT, Roman Kennke wrote: > Hi all, > > This pull request contains a backport of commit > [8eb453ba](https://github.com/openjdk/jdk/commit/8eb453baebe377697286f7eb32280ca9f1fd7775) > from the [openjdk/jdk](https://git.openjdk.java.net/jdk) repository. > > The commit being backported was authored by Roman Kennke on 10 Dec 2021 and > was reviewed by Roger Riggs and Peter Levart. > > It fixes a memory leak in ObjectStreamClass under certain conditions. See bug > for details. > > Thanks! Oops, I believe this should have been done vs jdk18u (not jdk18). Trying again. - PR: https://git.openjdk.java.net/jdk18/pull/117
[jdk18] RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive
Hi all, This pull request contains a backport of commit [8eb453ba](https://github.com/openjdk/jdk/commit/8eb453baebe377697286f7eb32280ca9f1fd7775) from the [openjdk/jdk](https://git.openjdk.java.net/jdk) repository. The commit being backported was authored by Roman Kennke on 10 Dec 2021 and was reviewed by Roger Riggs and Peter Levart. It fixes a memory leak in ObjectStreamClass under certain conditions. See bug for details. Thanks! - Commit messages: - Backport 8eb453baebe377697286f7eb32280ca9f1fd7775 Changes: https://git.openjdk.java.net/jdk18/pull/117/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk18=117=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277072 Stats: 496 lines in 4 files changed: 284 ins; 186 del; 26 mod Patch: https://git.openjdk.java.net/jdk18/pull/117.diff Fetch: git fetch https://git.openjdk.java.net/jdk18 pull/117/head:pull/117 PR: https://git.openjdk.java.net/jdk18/pull/117
Re: RFR: 8279917: Refactor subclassAudits in Thread to use ClassValue [v2]
On Thu, 13 Jan 2022 12:19:03 GMT, Roman Kennke wrote: >> Thread.java would benefit from a refactoring similar to JDK-8278065 to use >> ClassValue instead of the somewhat problematic WeakClassKey mechanism. >> >> Testing: >> - [x] tier1 >> - [x] tier2 >> - [x] tier3 > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Remove obsolete comment Thank you all! - PR: https://git.openjdk.java.net/jdk/pull/7054
Integrated: 8279917: Refactor subclassAudits in Thread to use ClassValue
On Wed, 12 Jan 2022 19:39:29 GMT, Roman Kennke wrote: > Thread.java would benefit from a refactoring similar to JDK-8278065 to use > ClassValue instead of the somewhat problematic WeakClassKey mechanism. > > Testing: > - [x] tier1 > - [x] tier2 > - [x] tier3 This pull request has now been integrated. Changeset: ce71e8b2 Author:Roman Kennke URL: https://git.openjdk.java.net/jdk/commit/ce71e8b281176d39cc879ae4ecf95f3d643ebd29 Stats: 86 lines in 1 file changed: 1 ins; 78 del; 7 mod 8279917: Refactor subclassAudits in Thread to use ClassValue Reviewed-by: alanb, rriggs - PR: https://git.openjdk.java.net/jdk/pull/7054
Re: RFR: 8279917: Refactor subclassAudits in Thread to use ClassValue [v2]
On Thu, 13 Jan 2022 14:41:51 GMT, Roger Riggs wrote: >> Roman Kennke has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove obsolete comment > > @AlanBateman has been doing a lot of work with j.l.Thread and Loom but may > not get to review this til next week. Thanks Ping? Can I please get another review? Maybe @RogerRiggs or @plevart, you already have been involved in similar changes in java.io ? Thanks, Roman - PR: https://git.openjdk.java.net/jdk/pull/7054
Re: RFR: 8279917: Refactor subclassAudits in Thread to use ClassValue [v2]
On Wed, 26 Jan 2022 19:09:58 GMT, Alan Bateman wrote: > I think this looks okay. It's only used when running with a SecurityManager > so unlikely to be used in most deployments. Thanks, Alan! Could you also 'approve' this PR? Thanks, Roman - PR: https://git.openjdk.java.net/jdk/pull/7054
Re: RFR: 8280041: Retry loop issues in java.io.ClassCache [v3]
On Wed, 19 Jan 2022 08:25:55 GMT, Aleksey Shipilev wrote: >> JDK-8277072 introduced java.io.ClassCache, but there seem to be at least two >> issues with it: >> - The cache cannot disambiguate between cleared SoftReference and the >> accidental passing of `null` value; in that case, the retry loop would spin >> indefinitely; >> - If retry loop would spin several times, every time discovering a cleared >> SoftReference, it would create and register new SoftReference on the >> ReferenceQueue. However, it would not *drain* the RQ in the loop; in that >> case, we might have the unbounded garbage accumulating in RQ; >> >> Attention @rkennke, @plevart. >> >> Additional testing: >> - [x] Linux x86_64 fastdebug `java/io/ObjectStreamClass` >> - [x] Linux x86_64 fastdebug `tier1` >> - [x] Linux x86_64 fastdebug `tier2` >> - [x] Linux x86_64 fastdebug `tier3` > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Test summary Still looks good (even better!) to me. Thanks! - Marked as reviewed by rkennke (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7092
Re: RFR: 8280041: Retry loop issues in java.io.ClassCache
On Fri, 14 Jan 2022 22:26:31 GMT, Aleksey Shipilev wrote: > > One additional improvement would be to change r.get() == null to > > r.refersTo(null) to check for cleared reference, that avoids reviving the > > referent, e.g. in SATB GCs. I leave that up to you. > > I think that does not work, because we want to strongly reference the `val` > for eventual return. Ah right. Looks ok then! - PR: https://git.openjdk.java.net/jdk/pull/7092
Re: RFR: 8280041: Retry loop issues in java.io.ClassCache
On Fri, 14 Jan 2022 19:27:18 GMT, Aleksey Shipilev wrote: > JDK-8277072 introduced java.io.ClassCache, but there seem to be at least two > issues with it: > - The cache cannot disambiguate between cleared SoftReference and the > accidental passing of `null` value; in that case, the retry loop would spin > indefinitely; > - If retry loop would spin several times, every time discovering a cleared > SoftReference, it would create and register new SoftReference on the > ReferenceQueue. However, it would not *drain* the RQ in the loop; in that > case, we might have the unbounded garbage accumulating in RQ; > > Attention @rkennke, @plevart. > > Additional testing: > - [x] Linux x86_64 fastdebug `java/io/ObjectStreamClass` > - [x] Linux x86_64 fastdebug `tier1` > - [x] Linux x86_64 fastdebug `tier2` > - [ ] Linux x86_64 fastdebug `tier3` Marked as reviewed by rkennke (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7092
Re: RFR: 8280041: Retry loop issues in java.io.ClassCache
On Fri, 14 Jan 2022 19:27:18 GMT, Aleksey Shipilev wrote: > JDK-8277072 introduced java.io.ClassCache, but there seem to be at least two > issues with it: > - The cache cannot disambiguate between cleared SoftReference and the > accidental passing of `null` value; in that case, the retry loop would spin > indefinitely; > - If retry loop would spin several times, every time discovering a cleared > SoftReference, it would create and register new SoftReference on the > ReferenceQueue. However, it would not *drain* the RQ in the loop; in that > case, we might have the unbounded garbage accumulating in RQ; > > Attention @rkennke, @plevart. > > Additional testing: > - [x] Linux x86_64 fastdebug `java/io/ObjectStreamClass` > - [x] Linux x86_64 fastdebug `tier1` > - [ ] Linux x86_64 fastdebug `tier2` > - [ ] Linux x86_64 fastdebug `tier3` Right, good catches! It seems a bit weird that the constructors of Reference would accept null referents anyway, but it is like it is. *shrugs* One additional improvement would be to change r.get() == null to r.refersTo(null) to check for cleared reference, that avoids reviving the referent, e.g. in SATB GCs. I leave that up to you. - Marked as reviewed by rkennke (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7092
Re: RFR: 8279917: Refactor subclassAudits in Thread to use ClassValue [v2]
On Thu, 13 Jan 2022 10:02:04 GMT, Andrey Turbanov wrote: >> Roman Kennke has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove obsolete comment > > src/java.base/share/classes/java/lang/Thread.java line 1671: > >> 1669: /** cache of subclass security audit results */ >> 1670: /* Replace with ConcurrentReferenceHashMap when/if it appears in a >> future >> 1671: * release */ > > Is it still desirable to use `ConcurrentReferenceHashMap` as suggested in > comment? Or ClassValue is better choice? Hmm, no. The use of Reference here is to avoid strong binding to the Class key. However, we have seen weird interferences in ObjectStreamClass (see JDK-8277072) between this mechanism and class-unloading which prevents unloading the class sometimes (or at least, absent memory pressure that would trigger the GC to reclaim soft references). This code here in Thread.java is probably not affected by this problem, but ClassValue is still the cleaner way to establish a mapping between a Class and some value. I have removed the comment. - PR: https://git.openjdk.java.net/jdk/pull/7054
Re: RFR: 8279917: Refactor subclassAudits in Thread to use ClassValue [v2]
> Thread.java would benefit from a refactoring similar to JDK-8278065 to use > ClassValue instead of the somewhat problematic WeakClassKey mechanism. > > Testing: > - [x] tier1 > - [x] tier2 > - [x] tier3 Roman Kennke has updated the pull request incrementally with one additional commit since the last revision: Remove obsolete comment - Changes: - all: https://git.openjdk.java.net/jdk/pull/7054/files - new: https://git.openjdk.java.net/jdk/pull/7054/files/0e495c4f..958e4e84 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7054=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7054=00-01 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7054.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7054/head:pull/7054 PR: https://git.openjdk.java.net/jdk/pull/7054
RFR: 8279917: Refactor subclassAudits in Thread to use ClassValue
Thread.java would benefit from a refactoring similar to JDK-8278065 to use ClassValue instead of the somewhat problematic WeakClassKey mechanism. Testing: - [x] tier1 - [x] tier2 - [x] tier3 - Commit messages: - 8279917: Refactor subclassAudits in Thread to use ClassValue Changes: https://git.openjdk.java.net/jdk/pull/7054/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7054=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8279917 Stats: 84 lines in 1 file changed: 1 ins; 76 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/7054.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7054/head:pull/7054 PR: https://git.openjdk.java.net/jdk/pull/7054
Re: RFR: 8278065: Refactor subclassAudits to use ClassValue [v4]
On Wed, 12 Jan 2022 14:27:17 GMT, Roger Riggs wrote: > I would have appreciated a chance to review the latest changes. Oh, ok, I'm sorry. I will wait some more next time that I get into a similar situation. I hope you're ok with the changes? - PR: https://git.openjdk.java.net/jdk/pull/6637
Integrated: 8278065: Refactor subclassAudits to use ClassValue
On Wed, 1 Dec 2021 14:45:23 GMT, Roman Kennke wrote: > As a follow-up to #6375, this change refactors > java.io.ObjectInputStream.Caches#subclassAudits and > java.io.ObjectOutputStream.Caches#subclassAudits to use ClassValue instead of > SoftReference, similar to what we did in #6375 for > java.io.ObjectStreamClass.Caches#localDescs. Then we can now also remove the > common machinery java.io.ObjectStreamClass#processQueue and > java.io.ObjectStreamClass.WeakClassKey. > > Testing: > - [x] tier1 > - [x] tier2 > - [ ] tier3 This pull request has now been integrated. Changeset: 8fed8ab2 Author:Roman Kennke URL: https://git.openjdk.java.net/jdk/commit/8fed8ab29cae4f189f44609c23f116967eef6bdf Stats: 105 lines in 3 files changed: 2 ins; 89 del; 14 mod 8278065: Refactor subclassAudits to use ClassValue Reviewed-by: rriggs, plevart - PR: https://git.openjdk.java.net/jdk/pull/6637
Re: RFR: 8278065: Refactor subclassAudits to use ClassValue [v3]
On Wed, 12 Jan 2022 06:45:02 GMT, Peter Levart wrote: > I think this looks good. Reviewed. Only a minor nit if you think it would be > better, but not necessary if you don't. The following combo: > > ``` > Boolean result = Caches.subclassAudits.get(cl); > assert result != null; > ``` > > could be written as: > > ``` > boolean result = Caches.subclassAudits.get(cl); > ``` > > ...and it would give the same "message" to the reader. WDYT? No need for > another round of reviews if you change this. Right! I changed it. BTW, I noticed that Thread.java has a similar subclassAudits machinery with WeakClassKey, which would also benefit from using ClassValue instead. I filed JDK-8279917 to track it. - PR: https://git.openjdk.java.net/jdk/pull/6637
Re: RFR: 8278065: Refactor subclassAudits to use ClassValue [v4]
> As a follow-up to #6375, this change refactors > java.io.ObjectInputStream.Caches#subclassAudits and > java.io.ObjectOutputStream.Caches#subclassAudits to use ClassValue instead of > SoftReference, similar to what we did in #6375 for > java.io.ObjectStreamClass.Caches#localDescs. Then we can now also remove the > common machinery java.io.ObjectStreamClass#processQueue and > java.io.ObjectStreamClass.WeakClassKey. > > Testing: > - [x] tier1 > - [x] tier2 > - [ ] tier3 Roman Kennke has updated the pull request incrementally with two additional commits since the last revision: - Merge remote-tracking branch 'origin/JDK-8278065' into JDK-8278065 - Avoid assert on unboxing Boolean - Changes: - all: https://git.openjdk.java.net/jdk/pull/6637/files - new: https://git.openjdk.java.net/jdk/pull/6637/files/6b37d520..78bda4b8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6637=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6637=02-03 Stats: 4 lines in 2 files changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6637.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6637/head:pull/6637 PR: https://git.openjdk.java.net/jdk/pull/6637
Re: RFR: 8278065: Refactor subclassAudits to use ClassValue [v3]
On Fri, 10 Dec 2021 21:07:37 GMT, Roman Kennke wrote:
>> As a follow-up to #6375, this change refactors
>> java.io.ObjectInputStream.Caches#subclassAudits and
>> java.io.ObjectOutputStream.Caches#subclassAudits to use ClassValue instead
>> of SoftReference, similar to what we did in #6375 for
>> java.io.ObjectStreamClass.Caches#localDescs. Then we can now also remove the
>> common machinery java.io.ObjectStreamClass#processQueue and
>> java.io.ObjectStreamClass.WeakClassKey.
>>
>> Testing:
>> - [x] tier1
>> - [x] tier2
>> - [ ] tier3
>
> Roman Kennke has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains ten commits:
>
> - Merge branch 'master' into JDK-8278065
> - 8278065: Refactor subclassAudits to use ClassValue
> - Remove unused EntryFuture inner class from ObjectSteamClass
> - Merge remote-tracking branch 'jdk-plevart/JDK-8277072-peter' into
> JDK-8277072
> - Use ClassValue to solve JDK-8277072
> - Use ForceGC instead of System.gc()
> - Convert test to testng
> - Fix indentation of new testcase
> - 8277072: ObjectStreamClass caches keep ClassLoaders alive
I think we shoul
> ...well I tried to do that. And it is not so simple. Here's what I came up
> with:
>
> ```java
> public final class ClassFlags {
>
> private static final class AtomicByte {
> private static final VarHandle VALUE;
>
> static {
> try {
> VALUE =
> MethodHandles.lookup().findVarHandle(AtomicByte.class, "value", byte.class);
> } catch (NoSuchFieldException | IllegalAccessException e) {
> throw new InternalError(e);
> }
> }
>
> private volatile byte value;
>
> byte get() {
> return value;
> }
>
> byte compareAndExchange(byte expectedValue, byte newValue) {
> return (byte) VALUE.compareAndExchange(this, (byte)
> expectedValue, (byte) newValue);
> }
> }
>
> private final Predicate>[] typePredicates;
> private final ClassValue flagsCV = new ClassValue<>() {
> @Override
> protected AtomicByte computeValue(Class type) {
> return new AtomicByte();
> }
> };
>
> @SafeVarargs
> public ClassFlags(Predicate>... typePredicates) {
> if (typePredicates.length > 4) {
> throw new IllegalArgumentException("Up to 4 flags are supported
> in a single ClassFlags instance");
> }
> this.typePredicates = typePredicates;
> }
>
> public boolean get(Class type, int index) {
> Objects.checkIndex(index, typePredicates.length);
>
> AtomicByte flags = flagsCV.get(type);
> int f = flags.get() & 0xFF;
> int falseMask = 0x1 << (index + index);
> int trueMask = falseMask << 1;
> int mask = falseMask | trueMask;
> int value = 0;
> while ((f & mask) == 0) {
> if (value == 0) {
> value = typePredicates[index].test(type) ? trueMask :
> falseMask;
> }
> int fn = (f & ~mask) | value;
> int fv = flags.compareAndExchange((byte) f, (byte) fn) & 0xFF;
> if (fv == f) {
> f = fn;
> break;
> } else {
> f = fv;
> }
> }
> return (f & trueMask) != 0;
> }
> }
> ```
>
> So I don't know if it's worth it... Simplicity of your implementation
> probably out-weights the footprint savings of above code.
>
> Regards, Peter
I think I'd rather keep it simple. Can you please give it a review, too?
Thanks, Roman
-
PR: https://git.openjdk.java.net/jdk/pull/6637
Re: RFR: 8278065: Refactor subclassAudits to use ClassValue [v3]
> As a follow-up to #6375, this change refactors > java.io.ObjectInputStream.Caches#subclassAudits and > java.io.ObjectOutputStream.Caches#subclassAudits to use ClassValue instead of > SoftReference, similar to what we did in #6375 for > java.io.ObjectStreamClass.Caches#localDescs. Then we can now also remove the > common machinery java.io.ObjectStreamClass#processQueue and > java.io.ObjectStreamClass.WeakClassKey. > > Testing: > - [x] tier1 > - [x] tier2 > - [ ] tier3 Roman Kennke has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains ten commits: - Merge branch 'master' into JDK-8278065 - 8278065: Refactor subclassAudits to use ClassValue - Remove unused EntryFuture inner class from ObjectSteamClass - Merge remote-tracking branch 'jdk-plevart/JDK-8277072-peter' into JDK-8277072 - Use ClassValue to solve JDK-8277072 - Use ForceGC instead of System.gc() - Convert test to testng - Fix indentation of new testcase - 8277072: ObjectStreamClass caches keep ClassLoaders alive - Changes: https://git.openjdk.java.net/jdk/pull/6637/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6637=02 Stats: 105 lines in 3 files changed: 2 ins; 87 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/6637.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6637/head:pull/6637 PR: https://git.openjdk.java.net/jdk/pull/6637
Re: RFR: 8278065: Refactor subclassAudits to use ClassValue [v2]
> As a follow-up to #6375, this change refactors > java.io.ObjectInputStream.Caches#subclassAudits and > java.io.ObjectOutputStream.Caches#subclassAudits to use ClassValue instead of > SoftReference, similar to what we did in #6375 for > java.io.ObjectStreamClass.Caches#localDescs. Then we can now also remove the > common machinery java.io.ObjectStreamClass#processQueue and > java.io.ObjectStreamClass.WeakClassKey. > > Testing: > - [x] tier1 > - [x] tier2 > - [ ] tier3 Roman Kennke has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - 8278065: Refactor subclassAudits to use ClassValue - Remove unused EntryFuture inner class from ObjectSteamClass - Merge remote-tracking branch 'jdk-plevart/JDK-8277072-peter' into JDK-8277072 - Use ClassValue to solve JDK-8277072 - Use ForceGC instead of System.gc() - Convert test to testng - Fix indentation of new testcase - 8277072: ObjectStreamClass caches keep ClassLoaders alive - Changes: https://git.openjdk.java.net/jdk/pull/6637/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6637=01 Stats: 431 lines in 4 files changed: 115 ins; 273 del; 43 mod Patch: https://git.openjdk.java.net/jdk/pull/6637.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6637/head:pull/6637 PR: https://git.openjdk.java.net/jdk/pull/6637
Integrated: 8277072: ObjectStreamClass caches keep ClassLoaders alive
On Fri, 12 Nov 2021 21:43:42 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 This pull request has now been integrated. Changeset: 8eb453ba Author:Roman Kennke URL: https://git.openjdk.java.net/jdk/commit/8eb453baebe377697286f7eb32280ca9f1fd7775 Stats: 496 lines in 4 files changed: 284 ins; 186 del; 26 mod 8277072: ObjectStreamClass caches keep ClassLoaders alive Reviewed-by: rriggs, plevart - 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 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 [v7]
On Tue, 7 Dec 2021 06:23:22 GMT, Peter Levart wrote:
>> Roman Kennke has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix cast; add testcase to cover memory pressure
>
> 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 {
> }
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.
-
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
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v6]
On Sat, 4 Dec 2021 08:47:03 GMT, Peter Levart wrote:
>> Roman Kennke has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove unnecessary import
>
> src/java.base/share/classes/java/io/ClassCache.java line 63:
>
>> 61: protected SoftReference computeValue(Class type) {
>> 62: return new
>> SoftReference<>(ClassCache.this.computeValue(type), queue);
>> 63: }
>
> How does this work? You create a bare SoftReference here and register it with
> queue
>
> ...then down in processQueue() you pick it up and cast to CacheRef... Doesn't
> this throw ClassCastException ?
>
Indeed, good catch! Apparently no test (existing or new) covers the
memory-pressure scenario. My new test only covers the scenario when the Class
disappears altogether. I added a test to verify behaviour under memory
pressure, and pushed a fix.
-
PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v7]
> 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: Fix cast; add testcase to cover memory pressure - Changes: - all: https://git.openjdk.java.net/jdk/pull/6375/files - new: https://git.openjdk.java.net/jdk/pull/6375/files/57428e0f..e6e34cea Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6375=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6375=05-06 Stats: 22 lines in 2 files changed: 18 ins; 0 del; 4 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
Re: RFR: 8278065: Refactor subclassAudits to use ClassValue
On Thu, 2 Dec 2021 19:49:12 GMT, Roger Riggs wrote: > Looks good. Thank you! I believe it makes sense to also use the ClassCache here, just as I did in #6375. I'll change this PR accordingly tomorrow. - PR: https://git.openjdk.java.net/jdk/pull/6637
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v5]
On Thu, 2 Dec 2021 16:14:18 GMT, Roger Riggs wrote: >> Roman Kennke has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Implement ClassCache: reclaim entries under memory pressure > > ObjectStreamClass may have an unnecesary import of `SoftReference`. > > Otherwise, looks good to me. Thanks, @RogerRiggs! @plevart does that also look reasonable to you? - PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v6]
> 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 unnecessary import - Changes: - all: https://git.openjdk.java.net/jdk/pull/6375/files - new: https://git.openjdk.java.net/jdk/pull/6375/files/570fac15..57428e0f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6375=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6375=04-05 Stats: 1 line in 1 file changed: 0 ins; 1 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
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 [v5]
> 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: Implement ClassCache: reclaim entries under memory pressure - Changes: - all: https://git.openjdk.java.net/jdk/pull/6375/files - new: https://git.openjdk.java.net/jdk/pull/6375/files/b7d53adc..570fac15 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6375=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6375=03-04 Stats: 93 lines in 2 files changed: 89 ins; 0 del; 4 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
RFR: 8278065: Refactor subclassAudits to use ClassValue
As a follow-up to #6375, this change refactors java.io.ObjectInputStream.Caches#subclassAudits and java.io.ObjectOutputStream.Caches#subclassAudits to use ClassValue instead of SoftReference, similar to what we did in #6375 for java.io.ObjectStreamClass.Caches#localDescs. Then we can now also remove the common machinery java.io.ObjectStreamClass#processQueue and java.io.ObjectStreamClass.WeakClassKey. Testing: - [ ] tier1 - [ ] tier2 - [ ] tier3 - Depends on: https://git.openjdk.java.net/jdk/pull/6375 Commit messages: - 8278065: Refactor subclassAudits to use ClassValue Changes: https://git.openjdk.java.net/jdk/pull/6637/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6637=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8278065 Stats: 105 lines in 3 files changed: 2 ins; 87 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/6637.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6637/head:pull/6637 PR: https://git.openjdk.java.net/jdk/pull/6637
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v3]
On Wed, 1 Dec 2021 10:49:54 GMT, Peter Levart wrote: > ...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 Thanks! - 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
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v2]
On Mon, 15 Nov 2021 21:35:06 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 two additional > commits since the last revision: > > - Use ForceGC instead of System.gc() > - Convert test to testng > A patch is worth a thousand words. Here's what I meant when I said this could > be elegantly solved with ClassValue: > > [plevart@6e16e5e](https://github.com/plevart/jdk/commit/6e16e5e526c7f3d868b16543f2f3418c751068e4) > > 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 successfully. I'm testing it more. 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? - PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v3]
> 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Merge remote-tracking branch 'jdk-plevart/JDK-8277072-peter' into JDK-8277072 - Use ClassValue to solve JDK-8277072 - Use ForceGC instead of System.gc() - Convert test to testng - Fix indentation of new testcase - 8277072: ObjectStreamClass caches keep ClassLoaders alive - Changes: - all: https://git.openjdk.java.net/jdk/pull/6375/files - new: https://git.openjdk.java.net/jdk/pull/6375/files/6f099c9c..2ed745b3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6375=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6375=01-02 Stats: 943784 lines in 2433 files changed: 495164 ins; 434236 del; 14384 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
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v2]
On Mon, 15 Nov 2021 21:35:06 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 two additional > commits since the last revision: > > - Use ForceGC instead of System.gc() > - Convert test to testng > 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? - PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v2]
On Mon, 15 Nov 2021 21:35:06 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 two additional > commits since the last revision: > > - Use ForceGC instead of System.gc() > - Convert test to testng Ping? Can I please get another review? Thanks! - PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v2]
On Mon, 15 Nov 2021 19:30:54 GMT, Roger Riggs wrote: >> Roman Kennke has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Use ForceGC instead of System.gc() >> - Convert test to testng > > test/jdk/java/io/ObjectStreamClass/TestOSCClassLoaderLeak.java line 55: > >> 53: con = null; >> 54: assert myOwnClassLoaderWeakReference.get() != null; >> 55: > > It is preferable is to write (new) tests using TestNG. > Relying on Assert to be enabled is not reliable. > It is preferable to make the checks explicit and throw RuntimeExceptions on > failure. Ok, I've changed to TestNG. Even though I often find that it's easier to debug a problem with a simple main method, instead of figuring out how to run the test in TestNG. > test/jdk/java/io/ObjectStreamClass/TestOSCClassLoaderLeak.java line 57: > >> 55: >> 56: gc(); >> 57: > > Is the dependency on ParallelGC necessary? > To may understanding invoking System.gc() is only a request to gc and does > not reflect any idea that it has completed. > There is a function in the test library util/ForceGC to ensure gc has > completed. ParallelGC is not necessary, this was a left-over from my own testing. I've removed it. I've also changed to use ForceGC, I did not know that it exists :-) - PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v2]
> 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 two additional commits since the last revision: - Use ForceGC instead of System.gc() - Convert test to testng - Changes: - all: https://git.openjdk.java.net/jdk/pull/6375/files - new: https://git.openjdk.java.net/jdk/pull/6375/files/430f54e5..6f099c9c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6375=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6375=00-01 Stats: 21 lines in 1 file changed: 8 ins; 7 del; 6 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
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive
On Mon, 15 Nov 2021 18:01:04 GMT, Joe Darcy wrote: > 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'. - PR: https://git.openjdk.java.net/jdk/pull/6375
RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive
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 - [ ] tier3 - [ ] tier4 - Commit messages: - Fix indentation of new testcase - 8277072: ObjectStreamClass caches keep ClassLoaders alive Changes: https://git.openjdk.java.net/jdk/pull/6375/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6375=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277072 Stats: 112 lines in 2 files changed: 107 ins; 1 del; 4 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
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v2]
On Fri, 13 Aug 2021 14:41:54 GMT, Alan Bateman wrote: > > Is this what you have been asking @mkarg in #4263 to do? Optimize > > transferTo() only for FileInputStream? Would it interfere with #4263? > > #4263 is the input stream returned by Channels.newInputStream where the > source may be a FileChannel and the output stream specified to > InputStream::transferTo may be connected to a FileChannel. Having followed both #4263 and this PR from the sidelines, may I ask why for #4263 much more rigorous testing is asked but not here? E.g. test for NPE, random-sized files/buffers, random position, return value, comprehensive JMH tests to show performance, etc? I'm all for high quality standards and good test coverage, but why are we seemingly measuring with double standards? - PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8272297: FileInputStream should override transferTo() for better performance [v2]
On Thu, 12 Aug 2021 21:07:53 GMT, Brian Burkhalter wrote: >> Please consider this request to add an override >> `java.io.FileInputStream.transferTo(OutputStream)` with improved performance >> if the parameter is a `FileOutputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8272297: Set source position after FC.transferTo(); add test Is this what you have been asking @mkarg in #4263 to do? Optimize transferTo() only for FileInputStream? Would it interfere with #4263? - PR: https://git.openjdk.java.net/jdk/pull/5097
Re: RFR: 8132984: incorrect type for Reference.discovered [v3]
On Mon, 18 Jan 2021 23:42:08 GMT, Kim Barrett wrote: >> Please review this change which fixes the type of the private >> Reference.discovered field. It was Reference, but that's wrong because >> it can be any Reference object. >> >> I've changed it to Reference and let that flow through, updating some >> other variables that were previously somewhat incorrectly typed (usually >> with an Object type parameter). The interesting change is to the >> ReferenceQueue.enqueue parameter, which is now also Reference. >> >> This ultimately end up with a provably safe and correct, but uncheckable, >> cast in ReferenceQueue.enqueue. >> >> An alternative might be to use a raw type for the discovered field, but I >> think that ends up with more @SuppressWarnings of various flavors. I think >> the unbounded wildcard approach is clearer and cleaner. >> >> Note that all of the pending list handling, including the discovered field, >> could be moved into a non-public, non-generic, sealed(?) base class of >> Reference. The pending list handling has nothing to do with the generic >> parameter T. >> >> Testing: >> mach5 tier1 and tier4 (tier4 is where vmTestbase_vm_gc_ref tests are run) > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > plevart improvement Looks good to me! - Marked as reviewed by rkennke (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1897
Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo [v2]
On Wed, 25 Nov 2020 23:35:14 GMT, Vladimir Kozlov wrote: >> JDK-8188055 added the function Reference.refersTo. For performance, the >> supporting native methods Reference.refersTo0 and PhantomReference.refersTo0 >> should be intrinsified by C2. >> >> Initial patch was prepared by @fisk. >> >> Tested hs-tier1-4. Added new compiler tests to test intrinsics. >> >> Ran new test with Shenandoah. Found only one issue. As result I disable >> PhantomReference::refersTo intrinsic for COOP+ Shenandoah combination. >> Someone from Shenandoah team have to test changes if that is enough. > > Vladimir Kozlov has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Merge branch 'master' into JDK-8256999 > - Added ZLoadBarrierElided = 0 definition. >Removed is_exact argument in load_field_from_object(). >Added Shenandoah support for narrow phantom accesses. > - 8256999: Add C2 intrinsic for Reference.refersTo and > PhantomReference::refersTo Shenandoah parts look good to me! Thanks! - Marked as reviewed by rkennke (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1425
Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification
On Mon, 23 Nov 2020 19:18:05 GMT, Per Liden wrote: >> test/hotspot/jtreg/gc/TestReferenceClearDuringReferenceProcessing.java line >> 28: >> >>> 26: /* @test >>> 27: * @bug 8256517 >>> 28: * @requires vm.gc.Z >> >> Please add | vm.gc.Shenandoah here. > > Note that for this test to be useful, the GC needs to support concurrent GC > breakpoints, which Shenandoah doesn't do. Ok, right. Nevermind then! - PR: https://git.openjdk.java.net/jdk/pull/1376
Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification
On Mon, 23 Nov 2020 01:43:39 GMT, Kim Barrett wrote: > Please review this change to Reference.clear() to address several issues. > > (JDK-8240696) For GCs using a SATB barrier, simply assigning the referent > field to null may extend the lifetime of the referent value. > > (JDK-8240696) For GCs with concurrent reference processing, clearing the > referent field during reference processing may discard the expected > notification. > > Both of these are addressed by introducing a private native helper function > for clearing the referent, rather than using an ordinary in-Java field > assignment. Tests have been added for both of these issues. This required > adding a new breakpoint in reference processing for ZGC. > > Of course, finalization adds some complexity to the problem. We deal with > that by having FinalReference override clear. The implementation is > provided by a new package-private method in Reference. (There are a number > of alternatives, all of them clumsy; finalization is annoying that way.) > > While dealing with FinalReference clearing it was noted that the recent > JDK-8256106 and JDK-8256370 have some problems. FinalizerHistogram was not > updated to call the new Reference.getInactive(), instead still calling get() > on FinalReferences, with the JDK-8256106 problems. Fixing that showed the > assertion for inactive FinalReference added by JDK-8256370 used the wrong > test. > > Rather than tracking down and changing all get() and clear() calls on final > references and changing them to use getInactive and a new similar clear > function, I've changed FinalReference to override get and clear, which call > the helper functions in Reference. I've also renamed getInactive to be more > explanatory and less convenient to call directly, and similarly named the > helper for clear. This means that get/clear should never be called on an > active FinalReference. That's already never done, and would have problems > if it were. > > Testing: > mach5 tier1-6 > Local (linux-x64) tier1 using Shenandoah. > New TestReferenceClearDuringMarking fails for G1 without these changes. > New TestReferenceClearDuringReferenceProcessing fails for ZGC without these > changes. Looks good! Thanks! - Marked as reviewed by rkennke (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1376
Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification
On Mon, 23 Nov 2020 01:43:39 GMT, Kim Barrett wrote: > Please review this change to Reference.clear() to address several issues. > > (JDK-8240696) For GCs using a SATB barrier, simply assigning the referent > field to null may extend the lifetime of the referent value. > > (JDK-8240696) For GCs with concurrent reference processing, clearing the > referent field during reference processing may discard the expected > notification. > > Both of these are addressed by introducing a private native helper function > for clearing the referent, rather than using an ordinary in-Java field > assignment. Tests have been added for both of these issues. This required > adding a new breakpoint in reference processing for ZGC. > > Of course, finalization adds some complexity to the problem. We deal with > that by having FinalReference override clear. The implementation is > provided by a new package-private method in Reference. (There are a number > of alternatives, all of them clumsy; finalization is annoying that way.) > > While dealing with FinalReference clearing it was noted that the recent > JDK-8256106 and JDK-8256370 have some problems. FinalizerHistogram was not > updated to call the new Reference.getInactive(), instead still calling get() > on FinalReferences, with the JDK-8256106 problems. Fixing that showed the > assertion for inactive FinalReference added by JDK-8256370 used the wrong > test. > > Rather than tracking down and changing all get() and clear() calls on final > references and changing them to use getInactive and a new similar clear > function, I've changed FinalReference to override get and clear, which call > the helper functions in Reference. I've also renamed getInactive to be more > explanatory and less convenient to call directly, and similarly named the > helper for clear. This means that get/clear should never be called on an > active FinalReference. That's already never done, and would have problems > if it were. > > Testing: > mach5 tier1-6 > Local (linux-x64) tier1 using Shenandoah. > New TestReferenceClearDuringMarking fails for G1 without these changes. > New TestReferenceClearDuringReferenceProcessing fails for ZGC without these > changes. Looks good, except one small nit in one of the test configs. test/hotspot/jtreg/gc/TestReferenceClearDuringReferenceProcessing.java line 28: > 26: /* @test > 27: * @bug 8256517 > 28: * @requires vm.gc.Z Please add | vm.gc.Shenandoah here. - Changes requested by rkennke (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1376
Re: RFR: 8256370: Add asserts to Reference.getInactive()
On Sun, 22 Nov 2020 22:15:20 GMT, Kim Barrett wrote: > I didn't notice this before it was integrated. > > The test for inactive isn't right; rather than `next == this` it > should be `next != null`. This becomes apparent once > FinalizerHistogram is fixed to call getInactive() rather than get(). > > I noticed this while working on JDK-8256517, where I ran into some > similar issues. I will address these problems as part of that change. Oh ok. Thanks for taking care of it! - PR: https://git.openjdk.java.net/jdk/pull/1231
Integrated: 8256370: Add asserts to Reference.getInactive()
On Mon, 16 Nov 2020 17:29:20 GMT, Roman Kennke wrote: > A follow-up to JDK-8256106, this is adding two asserts to check that the API > is used as it should be, i.e. only on inactive FinalReferences. Also, in > Finalizer, where getInactive() is used, there is a null-check. The GC must > never clean the referent, and Java code doesn't clean it either, it would be > a bug if we ever see null there. I think it's better to fail there (with > assert or NPE) when that happens instead of silently accepting it. > > Testing: > - [x] tier1 > - [x] tier2 This pull request has now been integrated. Changeset: f2a9d02d Author:Roman Kennke URL: https://git.openjdk.java.net/jdk/commit/f2a9d02d Stats: 4 lines in 2 files changed: 3 ins; 0 del; 1 mod 8256370: Add asserts to Reference.getInactive() Reviewed-by: mchung - PR: https://git.openjdk.java.net/jdk/pull/1231
RFR: 8256370: Add asserts to Reference.getInactive()
A follow-up to JDK-8256106, this is adding two asserts to check that the API is used as it should be, i.e. only on inactive FinalReferences. Also, in Finalizer, where getInactive() is used, there is a null-check. The GC must never clean the referent, and Java code doesn't clean it either, it would be a bug if we ever see null there. I think it's better to fail there (with assert or NPE) when that happens instead of silently accepting it. Testing: - [x] tier1 - [x] tier2 - Commit messages: - 8256370: Add asserts to Reference.getInactive() Changes: https://git.openjdk.java.net/jdk/pull/1231/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1231=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256370 Stats: 4 lines in 2 files changed: 3 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1231.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1231/head:pull/1231 PR: https://git.openjdk.java.net/jdk/pull/1231
Re: RFR: 8256106: Bypass intrinsic/barrier when calling Reference.get() from Finalizer [v3]
On Wed, 11 Nov 2020 18:13:30 GMT, Albert Mingkun Yang wrote: > With `getInactive`, is the null check, `if (finalizee != null` still needed? Good point! I don't think it is. The GC should not clean the referent before we finalized it (or not at all), and no other code is clearing it either. Unfortunately, I just integrated this PR, do you think it'd be worth to open a follow-up issue? - PR: https://git.openjdk.java.net/jdk/pull/1140
Re: RFR: 8256106: Bypass intrinsic/barrier when calling Reference.get() from Finalizer [v3]
On Wed, 11 Nov 2020 18:19:01 GMT, Mandy Chung wrote:
>> Roman Kennke has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Rename inactive-getter and improve javadocs
>
> src/java.base/share/classes/java/lang/ref/Reference.java line 356:
>
>> 354: */
>> 355: T getInactive() {
>> 356: return this.referent;
>
> It would be good to add `assert this instanceof FinalReference` to make this
> assertion clear.
Right. And maybe also assert that the Reference is indeed inactive. I'll open a
new issue for that (I already integrated this one, sorry I kinda jumped the gun
a little.)
-
PR: https://git.openjdk.java.net/jdk/pull/1140
Integrated: 8256106: Bypass intrinsic/barrier when calling Reference.get() from Finalizer
On Tue, 10 Nov 2020 09:37:29 GMT, Roman Kennke wrote: > Finalizer calls Reference.get() from the Finalizer to acquire the finalizee. > Concurrent reference processing GCs like Shenandoah and ZGC would return NULL > for unreachable referents, and thus would not call finalize() on them. > > ZGC works around this by fixing the referent before enqueuing, so that the > barrier would take the fast-path, but Shenandoah cannot do this. > > It is ok to bypass the barrier altogether in this place, because the > FinalReference is inactive and marking and reference-discovery treat inactive > FinalReferences like strong references. > > Testing: > - [x] hotspot_gc_shenandoah > - [x] tier1 +UseShenandoahGC +ShenandoahVerify > - [x] tier2 +UseShenandoahGC +ShenandoahVerify > - [x] tier1 > - [x] tier2 This pull request has now been integrated. Changeset: 96e02610 Author:Roman Kennke URL: https://git.openjdk.java.net/jdk/commit/96e02610 Stats: 15 lines in 2 files changed: 14 ins; 0 del; 1 mod 8256106: Bypass intrinsic/barrier when calling Reference.get() from Finalizer Reviewed-by: eosterlund - PR: https://git.openjdk.java.net/jdk/pull/1140
Re: RFR: 8256106: Bypass intrinsic/barrier when calling Reference.get() from Finalizer [v3]
> Finalizer calls Reference.get() from the Finalizer to acquire the finalizee. > Concurrent reference processing GCs like Shenandoah and ZGC would return NULL > for unreachable referents, and thus would not call finalize() on them. > > ZGC works around this by fixing the referent before enqueuing, so that the > barrier would take the fast-path, but Shenandoah cannot do this. > > It is ok to bypass the barrier altogether in this place, because the > FinalReference is inactive and marking and reference-discovery treat inactive > FinalReferences like strong references. > > Testing: > - [x] hotspot_gc_shenandoah > - [x] tier1 +UseShenandoahGC +ShenandoahVerify > - [x] tier2 +UseShenandoahGC +ShenandoahVerify > - [x] tier1 > - [x] tier2 Roman Kennke has updated the pull request incrementally with one additional commit since the last revision: Rename inactive-getter and improve javadocs - Changes: - all: https://git.openjdk.java.net/jdk/pull/1140/files - new: https://git.openjdk.java.net/jdk/pull/1140/files/61878147..45e35e78 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1140=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1140=01-02 Stats: 12 lines in 2 files changed: 7 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/1140.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1140/head:pull/1140 PR: https://git.openjdk.java.net/jdk/pull/1140
Re: RFR: 8256106: Bypass intrinsic/barrier when calling Reference.get() from Finalizer [v2]
> Finalizer calls Reference.get() from the Finalizer to acquire the finalizee. > Concurrent reference processing GCs like Shenandoah and ZGC would return NULL > for unreachable referents, and thus would not call finalize() on them. > > ZGC works around this by fixing the referent before enqueuing, so that the > barrier would take the fast-path, but Shenandoah cannot do this. > > It is ok to bypass the barrier altogether in this place, because the > FinalReference is inactive and marking and reference-discovery treat inactive > FinalReferences like strong references. > > Testing: > - [x] hotspot_gc_shenandoah > - [x] tier1 +UseShenandoahGC +ShenandoahVerify > - [x] tier2 +UseShenandoahGC +ShenandoahVerify > - [x] tier1 > - [x] tier2 Roman Kennke has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge branch 'master' into JDK-8256106 - 8256106: Bypass intrinsic/barrier when calling Reference.get() from Finalizer - Changes: - all: https://git.openjdk.java.net/jdk/pull/1140/files - new: https://git.openjdk.java.net/jdk/pull/1140/files/52820f98..61878147 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1140=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1140=00-01 Stats: 5646 lines in 150 files changed: 3822 ins; 862 del; 962 mod Patch: https://git.openjdk.java.net/jdk/pull/1140.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1140/head:pull/1140 PR: https://git.openjdk.java.net/jdk/pull/1140
RFR: 8256106: Bypass intrinsic/barrier when calling Reference.get() from Finalizer
Finalizer calls Reference.get() from the Finalizer to acquire the finalizee. Concurrent reference processing GCs like Shenandoah and ZGC would return NULL for unreachable referents, and thus would not call finalize() on them. ZGC works around this by fixing the referent before enqueuing, so that the barrier would take the fast-path, but Shenandoah cannot do this. It is ok to bypass the barrier altogether in this place, because the FinalReference is inactive and marking and reference-discovery treat inactive FinalReferences like strong references. Testing: - [x] hotspot_gc_shenandoah - [x] tier1 +UseShenandoahGC +ShenandoahVerify - [x] tier2 +UseShenandoahGC +ShenandoahVerify - [ ] tier1 - [ ] tier2 - Commit messages: - 8256106: Bypass intrinsic/barrier when calling Reference.get() from Finalizer Changes: https://git.openjdk.java.net/jdk/pull/1140/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1140=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256106 Stats: 8 lines in 2 files changed: 7 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1140.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1140/head:pull/1140 PR: https://git.openjdk.java.net/jdk/pull/1140
Re: RFR: 8249543: Force DirectBufferAllocTest to run with -ExplicitGCInvokesConcurrent
On Thu, 2020-07-16 at 08:09 +0100, Alan Bateman wrote: > On 15/07/2020 20:47, Roman Kennke wrote: > > : > > Why would it? Can you explain? (-ExplicitGCInvokesConcurrent is the > > default for all GCs but Shenandoah, and has been that way forever. > > Do > > you mean +ExplicitGCInvokesConcurrent?) > > > Just surprised that more tests aren't impacted. RMI DGC wouldn't > work > with a STW collector if explicit GC were disabled. Yeah, but +ExplicitGCInvokesConcurrent doesn't disable System.gc(), it only turns it into a concurrent cycle with the calling thread waiting for it to comlete. That is semantically very close to what STW System.gc() does. DirectBufferAllocTest is only problematic because it is not reliable as it is, and the added concurrency makes it worse, as far as I can tell. > I haven't heard of > deployment using it with a concurrent GC but maybe it's okay. I'm > just > surprised that the RMI tests in the jdk repo are robust enough to > pass, > I would have guessed they might need attention (the test group is > jdk_rmi but it sounds like you might be running those already). I've just run it again with my setup and it all passes. You ok with the patch? Thanks, Roman
Re: RFR: 8249543: Force DirectBufferAllocTest to run with -ExplicitGCInvokesConcurrent
Hi Alan, > On 15/07/2020 18:20, rken...@redhat.com wrote: > > DirectBufferAllocTest is unreliable when running with > > +ExplicitGCInvokesConcurrent, because allocating DBB spreads > > System.gc() calls all over concurrent GC cycles. It becomes more > > reliable when running with -ExplicitGCInvokesConcurrent. > > (Shenandoah > > defaults to +ExplicitGCInvokesConcurrent, other GCs don't as far as > > I > > know.) > > > > Bug: > > https://bugs.openjdk.java.net/browse/JDK-8249543 > > Webrev: > > http://cr.openjdk.java.net/~rkennke/JDK-8249543/webrev.00/ > > > > Ok? > > > I guess this is okay but if -ExplicitGCInvokesConcurrent is the > default > then doesn't it break RMI DGC? Why would it? Can you explain? (-ExplicitGCInvokesConcurrent is the default for all GCs but Shenandoah, and has been that way forever. Do you mean +ExplicitGCInvokesConcurrent?) Here's some context from our perspective: Normally, when System.gc() is called, it invokes a STW garbage collection. For most GCs that has been that way forever. This is what -ExplicitGCInvokesConcurrent implies. In Shenandoah, we opted to do +ExplicitGCInvokesConcurrent instead. This means that when System.gc() is called, a *concurrent* collection cycle is started, and the calling thread will wait for that to complete (and other threads will keep on running - unless they also call System.gc() ). It breaks this test because all test threads are hammering the GC with System.gc(), the first one will trigger the start of a concurrent GC, and the other ones will line up while concurrent GC is running. This is normally ok. However, the test (or even DirectByteBuffer allocation routine in Bits.java) is also over-assuming that when System.gc() returns (and Cleaner thread did its thing), it could now allocate native memory. However, when lots of test threads are competing for this, the last one could already been outrun by the first ones that are rescheduled already. The additional concurrency introduced by concurrent GC, plus a bunch of wrinkles in our implementation (e.g. the cleaner can run concurrently with ongoing GC, and not after the GC as it would do with STW GC) makes this test spuriously fail with Shenandoah. Forcing it to -ExplicitGCInvokesConcurrent makes it more reliable. But as far as I can tell, the test is intrinsically unreliable, but I'm also not sure how it could be made better (or the DBB allocator even). > Are you sure this is the only test that > fails? So far, yes. Can you point me to specific tests that you would expect to fail? Roman
Re: RFR: JDK-8235767: Compilation failure caused by JDK-8212780: Packaging Tool Implementation
That fix looks good to me. Thanks! Roman > The revised webrev at [3] is confirmed by the submitter to fix the build. > > please review > > /Andy > > [3] http://cr.openjdk.java.net/~herrick/8235767/webrev.02/ > > On 12/11/2019 9:49 AM, Andy Herrick wrote: >> Please review simple jpackage fix for issue [1] at [2] >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8235767 >> >> [2] http://cr.openjdk.java.net/~herrick/8235767/webrev.01/ >> >> /Andy >> >
Re: RFR: 8213325: (props) Properties.loadFromXML does not fully comply with the spec
(adding core-libs-dev) Ping? > I just realized that this has still not been reviewed. Can I do anything > to move forward? > > Thanks, > Roman > >> I added one extra verification to the JAXP based properties parser to >> verify that no extra internal DTD is being supplied. As far as I can >> tell, the other checks that have been added to the ukit parser for this >> bug are already done by the JAXP parser, by validating the XML against >> the built-in DTD. >> >> Webrev: >> http://cr.openjdk.java.net/~rkennke/JDK-8213325/webrev.01/ >> >> This passes all relevant tests, and I also run tier1 w/o regressions. >> >> What do you think? >> >> Roman >> >>> This is a backport of: >>> >>> https://bugs.openjdk.java.net/browse/JDK-8227274 >>> >>> which is a backport of the original: >>> >>> https://bugs.openjdk.java.net/browse/JDK-8213325 >>> >>> It's mostly the original change with a few significant extra modifications: >>> >>> - >>> src/share/classes/sun/util/xml/META-INF/services/sun.util.spi.XmlPropertiesProvider >>> >>> is changed from: >>> sun.util.xml.PlatformXmlPropertiesProvider >>> >>> to: >>> jdk.internal.util.xml.BasicXmlPropertiesProvider >>> >>> The reason is that the fix is for the latter, but 8u uses the former by >>> default. As far as I understand, the latter uses the new slimmer parser, >>> while the former uses the fullblown XML parser, but I am not sure about >>> that. However, we need to consider this, because it might be a very >>> significant change. >>> >>> The alternative would be to port over the fixes to the other XML parser, >>> which I have no desire to do. >>> >>> - The (test) JAR file: >>> test/java/util/spi/ResourceBundleControlProvider/rbcontrolprovider.jar >>> >>> has been regenerated from the modified sources XmlRB.xml and XmlRB_ja.xml. >>> >>> This is not present in jdk11 and later. I think it's generated on the >>> fly and not checked-in. >>> >>> Webrev: >>> http://cr.openjdk.java.net/~rkennke/JDK-8213325/webrev.00/ >>> >>> Testing: New testcases are passing. No regressions in tier1 tests. >>> >>> Opinions? Can I get reviews? >>> >>> Thanks, >>> Roman >>> >> >
Re: RFR 8220238 : Enhancing j.l.Runtime/System::gc specification with an explicit 'no guarantee' statement
>> Any other comments on: >> "* Runs the garbage collector in the Java Virtual Machine. >> * >> * Calling this method suggests that the Java Virtual Machine >> * expend effort toward recycling unused objects in order to >> * make the memory they currently occupy available for reuse >> * by the Java Virtual Machine. > > The following two statements... > > 1st: >> * When control returns from the method call, the Java Virtual Machine >> * has made a best effort to reclaim space from all discarded objects. > > 2nd: >> * There is no guarantee that this effort will recycle any particular >> * number of unused objects, reclaim any particular amount of space, >> * *or complete* at any particular time, if *at all*. >> " > > ...makes one think that it is OK (by the spec) for System.gc() to never > complete. > > Could it rather be specified that System.gc() eventually completes? > +1 I was thinking the same. I think the intention is that GC may never actually complete, but System.gc() must be guaranteed to eventually return. Roman
Re: RFR (S) 8212178: Soft reference reclamation race in com.sun.xml.internal.stream.util.ThreadLocalBufferAllocator
I have verified that the program that failed before doesn't appear to fail anymore. The patch looks good to me. Thanks! Roman > Bug: > https://bugs.openjdk.java.net/browse/JDK-8212178 > > See the details in the bug. We are hitting this race when Shenandoah is > running in aggressive mode > and evacuates lots of objects all the time, processing references very often. > This makes weak > reference reclamation races very visible. > > Fix: > http://cr.openjdk.java.net/~shade/8212178/webrev.01/ > > I took the liberty of rewriting the entire method to make it more > straightforward. New code > guarantees we never return "null" from this method. > > Testing: failing tests, jdk:tier1, jdk-submit (running) > > Thanks, > -Aleksey >
Re: RFR: JDK-8211071: unpack.cpp fails to compile with statement has no effect [-Werror=unused-value]
Hi Christoph & Magnus, thanks for reviewing! Am 27.09.18 um 08:22 schrieb Langer, Christoph: > Hi Roman, > > this looks good to me. +1 > > Best regards > Christoph > >> -Original Message- >> From: build-dev On Behalf Of >> Roman Kennke >> Sent: Mittwoch, 26. September 2018 19:24 >> To: Magnus Ihse Bursie ; core-libs- >> d...@openjdk.java.net >> Cc: build-...@openjdk.java.net >> Subject: Re: RFR: JDK-8211071: unpack.cpp fails to compile with statement >> has no effect [-Werror=unused-value] >> >> Ping core-libs? >> >> Roman >> >> Am 25.09.18 um 11:06 schrieb Magnus Ihse Bursie: >>> >>>> 25 sep. 2018 kl. 10:21 skrev Roman Kennke : >>>> >>>> Not sure this is the correct list. Please redirect as appropriate. >>> >>> I believe core-libs is the appropriate place. Cc:d. >>> >>>> >>>> Please review the following proposed change: >>>> >>>> >>>> There are 3 asserts in unpack.cpp which check only constants, and which >>>> the compiler rejects as 'has no effect' (in fastdebug builds). This >>>> seems to be caused by: >>>> >>>> https://bugs.openjdk.java.net/browse/JDK-8211029 >>>> >>>> I propose to fix this by defining a STATIC_ASSERT which can evaluate and >>>> reject this at compile time: >>>> >>>> Webrev: >>>> http://cr.openjdk.java.net/~rkennke/JDK-8211071/webrev.00/ >>> >>> From a build perspective, this looks good. But please let someone from >> core-libs review also. >>> >>>> Bug: >>>> https://bugs.openjdk.java.net/browse/JDK-8211071 >>>> >>>> It might be useful to define the STATIC_ASSERT in a more central place >>>> to be used elsewhere too. For example, there is a similar #define in >>>> Hotspot's debug.hpp >>> >>> Unfortunately, we do not have a good place to share definitions like this >> across JDK libraries. :-( The need for that has struck me more than once. For >> some stuff, we mis-use jni.h. But it would definitely be out of line to add a >> STATIC_ASSERT there. >>> >>> /Magnus >>> >>>> >>>> Thanks, Roman >>>> >>> >
Re: RFR: JDK-8211071: unpack.cpp fails to compile with statement has no effect [-Werror=unused-value]
Ping core-libs? Roman Am 25.09.18 um 11:06 schrieb Magnus Ihse Bursie: > >> 25 sep. 2018 kl. 10:21 skrev Roman Kennke : >> >> Not sure this is the correct list. Please redirect as appropriate. > > I believe core-libs is the appropriate place. Cc:d. > >> >> Please review the following proposed change: >> >> >> There are 3 asserts in unpack.cpp which check only constants, and which >> the compiler rejects as 'has no effect' (in fastdebug builds). This >> seems to be caused by: >> >> https://bugs.openjdk.java.net/browse/JDK-8211029 >> >> I propose to fix this by defining a STATIC_ASSERT which can evaluate and >> reject this at compile time: >> >> Webrev: >> http://cr.openjdk.java.net/~rkennke/JDK-8211071/webrev.00/ > > From a build perspective, this looks good. But please let someone from > core-libs review also. > >> Bug: >> https://bugs.openjdk.java.net/browse/JDK-8211071 >> >> It might be useful to define the STATIC_ASSERT in a more central place >> to be used elsewhere too. For example, there is a similar #define in >> Hotspot's debug.hpp > > Unfortunately, we do not have a good place to share definitions like this > across JDK libraries. :-( The need for that has struck me more than once. For > some stuff, we mis-use jni.h. But it would definitely be out of line to add a > STATIC_ASSERT there. > > /Magnus > >> >> Thanks, Roman >> >
RE: Potential memory leak in java.util.logging.Level
Hi Chris, (Sorry for top-posting, my stupid email program cannot do proper quoting) I don't understand why the CR is a 'request for enhancement' with 'very low' priority. A memory leak should be a bug with at least high priority in my world view. I will see if I find time to implement a fix. **Roman -Original Message- From: Chris Hegarty [mailto:chris.hega...@oracle.com] Sent: 22 December 2010 17:35 To: Roman Kennke Cc: core-libs-dev@openjdk.java.net Subject: Re: Potential memory leak in java.util.logging.Level On 12/22/10 02:36 PM, Roman Kennke wrote: Hello, I believe java.util.logging.Level is potentially memory leaking. This can happen if an application defines its own subclass of Level and is loaded by its own classloader. Level's constructor adds a reference to the new instance of the subclass to its 'known' ArrayList (which is a static field). This instance is never removed from that list. And since this instance holds a reference to its classloader, this means that no classes loaded by that classloader can be unloaded. It could be argued that you should not do crazy stuff with classloaders, but this happens invariably when you deal with applets. I guess similar problems can arise on app servers as well. I wonder why Level's constructors need to be protected though. If they were public, and new Levels could be created simply by instantiating one, this whole problem would not exist. Then the Level.known list would only reference instances of its own class, which would be problematic, unless you create LOTS of levels, which is unlikely. I don't like static collection fields anyway... too prone to leaking. Is this a known problem? Is there a solution to this, except not creating custom log levels? How else could custom log levels be created under those circumstances that would not trigger this problem? This would appear to be a known issue, see CR 6992761 [1]. -Chris. [1] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6992761 Thanks, Roman This communication is for informational purposes only. It is not intended as an offer or solicitation for the purchase or sale of any financial instrument or as an official confirmation of any transaction. All market prices, data and other information are not warranted as to completeness or accuracy and are subject to change without notice. Any comments or statements made herein do not necessarily reflect those of JPMorgan Chase Co., its subsidiaries and affiliates. This transmission may contain information that is privileged, confidential, legally privileged, and/or exempt from disclosure under applicable law. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, or use of the information contained herein (including any reliance thereon) is STRICTLY PROHIBITED. Although this transmission and any attachments are believed to be free of any virus or other defect that might affect any computer system into which it is received and opened, it is the responsibility of the recipient to ensure that it is virus free and no responsibility is accepted by JPMorgan Chase Co., its subsidiaries and affiliates, as applicable, for any loss or damage arising in any way from its use. If you received this transmission in error, please immediately contact the sender and destroy the material in its entirety, whether in electronic or hard copy format. Thank you. Please refer to http://www.jpmorgan.com/pages/disclosures for disclosures relating to European legal entities.
Potential memory leak in java.util.logging.Level
Hello, I believe java.util.logging.Level is potentially memory leaking. This can happen if an application defines its own subclass of Level and is loaded by its own classloader. Level's constructor adds a reference to the new instance of the subclass to its 'known' ArrayList (which is a static field). This instance is never removed from that list. And since this instance holds a reference to its classloader, this means that no classes loaded by that classloader can be unloaded. It could be argued that you should not do crazy stuff with classloaders, but this happens invariably when you deal with applets. I guess similar problems can arise on app servers as well. I wonder why Level's constructors need to be protected though. If they were public, and new Levels could be created simply by instantiating one, this whole problem would not exist. Then the Level.known list would only reference instances of its own class, which would be problematic, unless you create LOTS of levels, which is unlikely. I don't like static collection fields anyway... too prone to leaking. Is this a known problem? Is there a solution to this, except not creating custom log levels? How else could custom log levels be created under those circumstances that would not trigger this problem? Thanks, Roman This communication is for informational purposes only. It is not intended as an offer or solicitation for the purchase or sale of any financial instrument or as an official confirmation of any transaction. All market prices, data and other information are not warranted as to completeness or accuracy and are subject to change without notice. Any comments or statements made herein do not necessarily reflect those of JPMorgan Chase Co., its subsidiaries and affiliates. This transmission may contain information that is privileged, confidential, legally privileged, and/or exempt from disclosure under applicable law. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, or use of the information contained herein (including any reliance thereon) is STRICTLY PROHIBITED. Although this transmission and any attachments are believed to be free of any virus or other defect that might affect any computer system into which it is received and opened, it is the responsibility of the recipient to ensure that it is virus free and no responsibility is accepted by JPMorgan Chase Co., its subsidiaries and affiliates, as applicable, for any loss or damage arising in any way from its use. If you received this transmission in error, please immediately contact the sender and destroy the material in its entirety, whether in electronic or hard copy format. Thank you. Please refer to http://www.jpmorgan.com/pages/disclosures for disclosures relating to European legal entities.
Re: malloc failures in java/util/zip/Deflater
Hi Mario, According to the specs, malloc may return either a valid pointer that can be passed to free, or NULL, while generally NULL is considered to be a failure. Linux and Solaris, albeit non specifying it, return always a valid pointer, as far as I know I think NULL is returned in an out of memory situation, which is very rare on modern OSes, but I think it's still possible. The right thing to do here is check for NULL and (try to) throw an OOME. Which is what is beeing done already (AFAICS). What are you trying to solve by additionally checking for len 0? /Roman Hi Roman, The OutOfMemory is thrown correctly in case of failure (it wasn't up to some builds ago, though :). The problem is when passing a 0 length argument to malloc (from the man page): malloc() allocates size bytes and returns a pointer to the allocated memory. The memory is not cleared. If size is 0, then malloc() returns either NULL, or a unique pointer value that can later be successfully passed to free(). Linux and Solaris AFAIK return a pointer to valid memory, but this is not specified, and the code only checks for NULL as in failure. So it may be the case that this changes in future. In my case I have a not-so-modern OS that returns NULL in such case. So, to decide if we have a memory error or not, we need the additional len 0 check. Ah, I see! The len==0 case is not a failure, but some OSes return NULL anyway, running into an OOME even when there's no error? Now that you don't throw OOME in these cases, you might want to check the pointer later so that you don't get a segfault when you pass this NULL pointer to other functions like free(). (uhm .. TARGET_FREE_UNSAFE? oops, internal joke... ;-). ) /Roman
[PATCH] Move Solaris specific classes to solaris/
Hi,
there are some classes in the jdk/share tree, that seem to be Solaris
specific. I suggest moving them to the jdk/solaris tree instead. Or am I
wrong here?
/Roman
--
http://kennke.org/blog/
# HG changeset patch
# User Roman Kennke [EMAIL PROTECTED]
# Date 1201293270 -3600
# Node ID db9384d2f46857b26ae306b4a0e1d25a049c634e
# Parent 2b6c2ce8cd88445d9e3ea709069bf26d53039223
Moved Solaris specific NIO Java classes to the solaris subdir
diff -r 2b6c2ce8cd88 -r db9384d2f468
src/share/classes/sun/nio/ch/AbstractPollSelectorImpl.java
--- a/src/share/classes/sun/nio/ch/AbstractPollSelectorImpl.javaTue Dec
18 15:30:58 2007 +0100
+++ /dev/null Thu Jan 01 00:00:00 1970 +
@@ -1,187 +0,0 @@
-/*
- * Copyright 2001-2004 Sun Microsystems, Inc. All Rights Reserved.
- * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
- *
- * This code is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License version 2 only, as
- * published by the Free Software Foundation. Sun designates this
- * particular file as subject to the Classpath exception as provided
- * by Sun in the LICENSE file that accompanied this code.
- *
- * This code is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
- * version 2 for more details (a copy is included in the LICENSE file that
- * accompanied this code).
- *
- * You should have received a copy of the GNU General Public License version
- * 2 along with this work; if not, write to the Free Software Foundation,
- * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
- *
- * Please contact Sun Microsystems, Inc., 4150 Network Circle, Santa Clara,
- * CA 95054 USA or visit www.sun.com if you need additional information or
- * have any questions.
- */
-
-package sun.nio.ch;
-
-import java.io.IOException;
-import java.nio.channels.*;
-import java.nio.channels.spi.*;
-import java.util.*;
-import sun.misc.*;
-
-
-/**
- * An abstract selector impl.
- */
-
-abstract class AbstractPollSelectorImpl
-extends SelectorImpl
-{
-
-// The poll fd array
-PollArrayWrapper pollWrapper;
-
-// Initial capacity of the pollfd array
-protected final int INIT_CAP = 10;
-
-// The list of SelectableChannels serviced by this Selector
-protected SelectionKeyImpl[] channelArray;
-
-// In some impls the first entry of channelArray is bogus
-protected int channelOffset = 0;
-
-// The number of valid channels in this Selector's poll array
-protected int totalChannels;
-
-// True if this Selector has been closed
-private boolean closed = false;
-
-AbstractPollSelectorImpl(SelectorProvider sp, int channels, int offset) {
-super(sp);
-this.totalChannels = channels;
-this.channelOffset = offset;
-}
-
-void putEventOps(SelectionKeyImpl sk, int ops) {
-pollWrapper.putEventOps(sk.getIndex(), ops);
-}
-
-public Selector wakeup() {
-pollWrapper.interrupt();
-return this;
-}
-
-protected abstract int doSelect(long timeout) throws IOException;
-
-protected void implClose() throws IOException {
-if (!closed) {
-closed = true;
-// Deregister channels
-for(int i=channelOffset; itotalChannels; i++) {
-SelectionKeyImpl ski = channelArray[i];
-assert(ski.getIndex() != -1);
-ski.setIndex(-1);
-deregister(ski);
-SelectableChannel selch = channelArray[i].channel();
-if (!selch.isOpen() !selch.isRegistered())
-((SelChImpl)selch).kill();
-}
-implCloseInterrupt();
-pollWrapper.free();
-pollWrapper = null;
-selectedKeys = null;
-channelArray = null;
-totalChannels = 0;
-}
-}
-
-protected abstract void implCloseInterrupt() throws IOException;
-
-/**
- * Copy the information in the pollfd structs into the opss
- * of the corresponding Channels. Add the ready keys to the
- * ready queue.
- */
-protected int updateSelectedKeys() {
-int numKeysUpdated = 0;
-// Skip zeroth entry; it is for interrupts only
-for (int i=channelOffset; itotalChannels; i++) {
-int rOps = pollWrapper.getReventOps(i);
-if (rOps != 0) {
-SelectionKeyImpl sk = channelArray[i];
-pollWrapper.putReventOps(i, 0);
-if (selectedKeys.contains(sk)) {
-if (sk.channel.translateAndSetReadyOps(rOps, sk)) {
-numKeysUpdated++;
-}
-} else {
-sk.channel.translateAndSetReadyOps(rOps, sk);
-if ((sk.nioReadyOps() sk.nioInterestOps
Re: Java should provide exact real arithmetics
Hi there, snip Currently, there exist much better solutions than BigDecimal, unfortunately not from the Java space. For instance GMP ( http://gmplib.org/) is considered as one of the state of the art libraries in the area of precision arithmetics. And several libraries extent GMP such as the popular iRRAM project (http://www.informatik.uni-trier.de/iRRAM/). Check out the following page in order to get an idea how performable GMP is: http://gmplib.org/pi-with-gmp.html it demonstrates the calculation of pi with many digits. I´m asking myself why there is nothing comparable for the java platfom. Have you tried: http://www.apfloat.org/ Not everything has to be in the core platform IMO (I think it's already much too bloated). Or maybe it could be considered to write a JNI-Layer on top of GMP? Cheers, Roman -- http://kennke.org/blog/
PlainSocketImpl.socketGetOption1() ?
Hi list, The PlainSocketImpl family of classes have a method socketGetOption1(), which is required by the AbstractPlainSocketImpl class, and implemented by the subclasses, but apparently never used. There isn't even a native implementation for the native method in PlainSocketImpl. Could this method be removed, or is there another story behind this? /Roman -- http://kennke.org/blog/
Bug#6546113 StringCharBuffer fix
This fixes a problem with sliced StringCharBuffers. See: http://bugs.sun.com/view_bug.do?bug_id=6546113 Feel free to integrate this in the repository. Cheers, Roman -- http://kennke.org/blog/
Re: Bug#6546113 StringCharBuffer fix
I definitely need this new Evolution thingy. Forgot the actual
attachement..
Am Donnerstag, den 27.09.2007, 20:50 +0200 schrieb Roman Kennke:
This fixes a problem with sliced StringCharBuffers. See:
http://bugs.sun.com/view_bug.do?bug_id=6546113
Feel free to integrate this in the repository.
Cheers, Roman
--
http://kennke.org/blog/
Index: j2se/src/share/classes/java/nio/StringCharBuffer.java
===
--- j2se/src/share/classes/java/nio/StringCharBuffer.java (Revision 252)
+++ j2se/src/share/classes/java/nio/StringCharBuffer.java (Arbeitskopie)
@@ -77,11 +77,11 @@
}
public final char get() {
- return str.charAt(nextGetIndex());
+ return str.charAt(nextGetIndex() + offset);
}
public final char get(int index) {
- return str.charAt(checkIndex(index));
+ return str.charAt(checkIndex(index) + offset);
}
// ## Override bulk get methods for better performance
