Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8278065: Refactor subclassAudits to use ClassValue #6637

Closed
wants to merge 11 commits into from

Conversation

rkennke
Copy link
Contributor

@rkennke rkennke commented Dec 1, 2021

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

Progress

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

Issue

  • JDK-8278065: Refactor subclassAudits to use ClassValue

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6637

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 1, 2021

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

@openjdk openjdk bot added the rfr label Dec 1, 2021
@openjdk
Copy link

openjdk bot commented Dec 1, 2021

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

  • core-libs

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

@openjdk openjdk bot added the core-libs label Dec 1, 2021
@mlbridge
Copy link

mlbridge bot commented Dec 1, 2021

Webrevs

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Looks good.

@rkennke
Copy link
Contributor Author

rkennke commented Dec 2, 2021

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.

@RogerRiggs
Copy link
Contributor

RogerRiggs commented Dec 2, 2021

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.

@rkennke
Copy link
Contributor Author

rkennke commented Dec 2, 2021

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.

Right. Ok thanks, let's wait for #6375 then.

@plevart
Copy link
Contributor

plevart commented Dec 8, 2021

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.

Right, SoftReference object has a bigger footprint than Boolean object (more fields). ObjectStreamClass was another beast. So it is actually detrimental to efficiency to keep Boolean inside SoftReference just to be able to release Boolean, since SoftReference will stay until associated Class or ClassValue goes away...

What you could do is to half the footprint of those two caches if you merge them both into a single cache with a value holding two primitive booleans. Such object has the same foorprint as Boolean (because of padding) and you only need one ClassValue instance which would halve the number of ClassValueMap entries in existence.

What do you think?

@plevart
Copy link
Contributor

plevart commented Dec 8, 2021

...well I tried to do that. And it is not so simple. Here's what I came up with:

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<? super Class<?>>[] typePredicates;
    private final ClassValue<AtomicByte> flagsCV = new ClassValue<>() {
        @Override
        protected AtomicByte computeValue(Class<?> type) {
            return new AtomicByte();
        }
    };

    @SafeVarargs
    public ClassFlags(Predicate<? super Class<?>>... 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

@openjdk-notifier openjdk-notifier bot changed the base branch from pr/6375 to master Dec 10, 2021
@openjdk-notifier
Copy link

openjdk-notifier bot commented Dec 10, 2021

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

@erikj79
Copy link
Member

erikj79 commented Dec 10, 2021

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 recently going in. If you merge jdk:master into your branch, I believe this will resolve itself for now.

@erikj79
Copy link
Member

erikj79 commented Dec 10, 2021

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 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.

@openjdk
Copy link

openjdk bot commented Dec 10, 2021

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

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

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

8278065: Refactor subclassAudits to use ClassValue

Reviewed-by: rriggs, plevart

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

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

  • ece98d8: 8278461: Use Executable.getSharedParameterTypes() instead of Executable.getParameterTypes() in trusted code
  • 525b20f: 8279676: Dubious YMM register clearing in x86_64 arraycopy stubs
  • 4f0b650: 8278581: Improve reference processing statistics log output
  • bd339aa: 8277627: Fix copyright years in some jvmci files
  • 319d230: 8277463: JFileChooser with Metal L&F doesn't show non-canonical UNC path in - Look in
  • 13bfb49: 6496103: isFileHidingEnabled return false by default
  • f16f6a9: 8279821: JFR: Log warnings properly when loading a misconfigured .jfc file
  • 1c688f4: 8279900: compiler/vectorization/TestPopCountVectorLong.java fails due to vpopcntdq is not supported
  • 3aaa098: 8276694: Pattern trailing unescaped backslash causes internal error
  • 36f41cb: 8279884: Use better file for cygwin source permission check
  • ... and 248 more: https://git.openjdk.java.net/jdk/compare/61736f81fb4a20375c83d59e2b37a00aafb11107...master

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

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

@openjdk
Copy link

openjdk bot commented Dec 10, 2021

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

@openjdk openjdk bot added the ready label Dec 10, 2021
@bridgekeeper
Copy link

bridgekeeper bot commented Jan 8, 2022

@rkennke This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@rkennke
Copy link
Contributor Author

rkennke commented Jan 10, 2022

I think we shoul

...well I tried to do that. And it is not so simple. Here's what I came up with:

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<? super Class<?>>[] typePredicates;
    private final ClassValue<AtomicByte> flagsCV = new ClassValue<>() {
        @Override
        protected AtomicByte computeValue(Class<?> type) {
            return new AtomicByte();
        }
    };

    @SafeVarargs
    public ClassFlags(Predicate<? super Class<?>>... 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

@plevart
Copy link
Contributor

plevart commented Jan 12, 2022

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.

@rkennke
Copy link
Contributor Author

rkennke commented Jan 12, 2022

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.

@rkennke
Copy link
Contributor Author

rkennke commented Jan 12, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Jan 12, 2022

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

  • f54ce84: 8238161: use os::fopen in HS code where possible
  • ff0cb98: 8279536: jdk/nio/zipfs/ZipFSOutputStreamTest.java timed out
  • ece98d8: 8278461: Use Executable.getSharedParameterTypes() instead of Executable.getParameterTypes() in trusted code
  • 525b20f: 8279676: Dubious YMM register clearing in x86_64 arraycopy stubs
  • 4f0b650: 8278581: Improve reference processing statistics log output
  • bd339aa: 8277627: Fix copyright years in some jvmci files
  • 319d230: 8277463: JFileChooser with Metal L&F doesn't show non-canonical UNC path in - Look in
  • 13bfb49: 6496103: isFileHidingEnabled return false by default
  • f16f6a9: 8279821: JFR: Log warnings properly when loading a misconfigured .jfc file
  • 1c688f4: 8279900: compiler/vectorization/TestPopCountVectorLong.java fails due to vpopcntdq is not supported
  • ... and 250 more: https://git.openjdk.java.net/jdk/compare/61736f81fb4a20375c83d59e2b37a00aafb11107...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Jan 12, 2022
@openjdk openjdk bot closed this Jan 12, 2022
@openjdk openjdk bot removed ready rfr labels Jan 12, 2022
@openjdk
Copy link

openjdk bot commented Jan 12, 2022

@rkennke Pushed as commit 8fed8ab.

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

@rkennke rkennke deleted the JDK-8278065 branch Jan 12, 2022
@RogerRiggs
Copy link
Contributor

RogerRiggs commented Jan 12, 2022

I would have appreciated a chance to review the latest changes.

@rkennke
Copy link
Contributor Author

rkennke commented Jan 12, 2022

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs integrated
4 participants