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
Re: RFR: 8278065: Refactor subclassAudits to use ClassValue [v4]
On Wed, 12 Jan 2022 11:15:56 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 incrementally with two additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/JDK-8278065' into JDK-8278065 > - Avoid assert on unboxing Boolean I would have appreciated a chance to review the latest changes. - 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 Marked as reviewed by plevart (Reviewer). 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. - 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]
On Fri, 10 Dec 2021 16:31:45 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 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 > Skara is failing to run jcheck on this PR. I think this is ultimately a bug > in Skara, that gets tickled by your source branch being quite far behind > jdk:master, in combination with > [62a7f5d](https://github.com/openjdk/jdk/commit/62a7f5d3236ab2248518a475b1d8b71cb4bf1313) > recently going in. If you merge jdk:master into your branch, I believe this > will resolve itself for now. I've investigated further and filed https://bugs.openjdk.java.net/browse/SKARA-1281. Once fixed you would get an error message in this PR telling you to rebase your source branch. - PR: https://git.openjdk.java.net/jdk/pull/6637
Re: RFR: 8278065: Refactor subclassAudits to use ClassValue [v2]
On Fri, 10 Dec 2021 16:31:45 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 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 Skara is failing to run jcheck on this PR. I think this is ultimately a bug in Skara, that gets tickled by your source branch being quite far behind jdk:master, in combination with 62a7f5d3236ab2248518a475b1d8b71cb4bf1313 recently going in. If you merge jdk:master into your branch, I believe this will resolve itself for now. - 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
Re: RFR: 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 The dependent pull request has now been integrated, and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork: git checkout JDK-8277072 git fetch https://git.openjdk.java.net/jdk master git merge FETCH_HEAD # if there are conflicts, follow the instructions given by git merge git commit -m "Merge master" git push - PR: https://git.openjdk.java.net/jdk/pull/6637
Re: RFR: 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 The ClassValues are simple Booleans, that won't make much of difference if there's memory pressure and the ClassCache adds more objects and overhead of processing the queue, so I don't think its beneficial. - PR: https://git.openjdk.java.net/jdk/pull/6637
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: 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 Looks good. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6637
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
