8292375: Convert ProtectionDomainCacheTable to ResourceHashtable #10043

coleenp · 2022-08-26T13:43:00Z

Please review this simple conversion for the ProtectionDomainCacheTable from Old Hashtable to ResourceHashtable. There are specific tests for this table in test/hotspot/jtreg/runtime/Dictionary and serviceability/dcmd/vm/DictionaryStatsTest.java.
Also tested with tier1-7.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8292375: Convert ProtectionDomainCacheTable to ResourceHashtable

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Ioi Lam (@iklam - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10043/head:pull/10043
$ git checkout pull/10043

Update a local copy of the PR:
$ git checkout pull/10043
$ git pull https://git.openjdk.org/jdk pull/10043/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10043

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10043.diff

bridgekeeper · 2022-08-26T13:44:32Z

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

openjdk · 2022-08-26T13:48:00Z

@coleenp The following labels will be automatically applied to this pull request:

hotspot
serviceability

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

mlbridge · 2022-08-26T13:52:31Z

Webrevs

dholmes-ora

Hi Coleen,

Probably due to my lack of understanding of the existing code I found this conversion very hard to follow. A number of comments below.

Thanks.

dholmes-ora · 2022-08-29T00:12:36Z

src/hotspot/share/classfile/protectionDomainCache.cpp

-unsigned int ProtectionDomainCacheTable::compute_hash(Handle protection_domain) {
-  // Identity hash can safepoint, so keep protection domain in a Handle.
-  return (unsigned int)(protection_domain->identity_hash());
+unsigned int ProtectionDomainCacheTable::compute_hash(const WeakHandle& protection_domain) {


Why are we now using WeakHandle everywhere?

WeakHandle is the object we're storing as the value in the hashtable. It also turns out to be the key.

src/hotspot/share/classfile/protectionDomainCache.cpp

coleenp · 2022-08-29T12:19:23Z

Thanks for reading through this. I was going to make a larger change, then changed my mind. This conversion is limited. It is able to take advantage of the ability to copy WeakHandle without side effects, so that we don't have to store ProtectionDomainCacheEntry into the pd_set linked list, which makes it nicer.

iklam

Looks good overall and seems to be equivalent to the old code. Just a couple of nits.

src/hotspot/share/classfile/protectionDomainCache.cpp

src/hotspot/share/classfile/protectionDomainCache.hpp

openjdk · 2022-08-30T19:32:12Z

@coleenp 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:

8292375: Convert ProtectionDomainCacheTable to ResourceHashtable

Reviewed-by: dholmes, iklam

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 85 new commits pushed to the master branch:

3ac91b0: 8293003: Review running time of Warn5 regression test
e0168a0: 8288012: AArch64: unnecessary macro expansion in stubGenerator_aarch64
99c3ab0: 8293010: JDI ObjectReference/referringObjects/referringObjects001 fails: assert(env->is_enabled(JVMTI_EVENT_OBJECT_FREE)) failed: checking
0fb9469: 8290126: Add a check in JavadocTester for "javadoc should not crash"
0a4d0ce: 8293121: (fs) Refactor UnixFileSystem copying into generic Unix, Linux, and BSD implementations
032be16: 8292066: Convert TestInputArgument.sh and TestSystemLoadAvg.sh to java version
e393973: 8292990: Improve test coverage for XPath Axes: parent
fa68371: 8292584: assert(cb != __null) failed: must be with -XX:-Inline
04d8069: 8230374: maxOutputSize, instead of javatest.maxOutputSize, should be used in TEST.properties
3d254d3: 8289510: Improve test coverage for XPath Axes: namespace
... and 75 more: https://git.openjdk.org/jdk/compare/95a33fe1502b6f0db2c60fa92b389fda74d94407...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.

coleenp

Thank you for reviewing, Ioi.

src/hotspot/share/classfile/protectionDomainCache.cpp

src/hotspot/share/classfile/protectionDomainCache.hpp

dholmes-ora · 2022-09-01T04:12:32Z

src/hotspot/share/classfile/protectionDomainCache.hpp

+// The ProtectionDomainCacheTable maps all java.security.ProtectionDomain objects that are
+// registered by DictionaryEntry::add_protection_domain() to a unique WeakHandle.


Now that I understand what this table does, this comment is confusing to me. The table maps each PD to itself (using the WeakHandle as the actual key and value) as a means to track which PDs have been seen/registered. I would describe it thus:

// The ProtectionDomainCacheTable records all of the java.security.ProtectionDomain instances that have // been registered by DictionaryEntry::add_protection_domain(). We use a hashtable to map each PD // instance to itself (using WeakHandles) to allow for fast lookup.

“Mapping to itself” is how this table works internally. I am not sure if this information is useful here. But the purpose of this table is really to keep track of all PDs that have been registered and not yet garbage collected.

"mapping to itself" is more useful than "mapping ... to a unique Weakhandle" - which is even more of an internal implementation detail. I found the use of this table very hard to discern based on the internal use of the hashtable, as there is no real mapping operation - we simply track if a PD has been seen or not. The use of the hashtable is purely for lookup convenience - we could instead have a linked-list of PD's that we traverse for lookup.
So perhaps we drop my second sentence above, and move it to where the hashtable itself is declared?

"mapping a PD to a unique Weakhandle" is not an implementation detail. It's the only useful API provided by this class:

WeakHandle obj = ProtectionDomainCacheTable::add_if_absent(protection_domain);

and that's the reason I question why this table is needed at all.

The fact it is a weakhandle is an implementation detail. The table simply records whether a PD (wrapped in a WeakHandle) has been seen.

Since I know what this table does, either comment is fine if it helps someone understand it.
The "map each PD to itself" is pretty odd to me too. How about collect each PD for fast lookup in a hashtable? The code says how it's mapped. There isn't that much code and it's easy to see how the Key is mapped.

// The ProtectionDomainCacheTable maps all java.security.ProtectionDomain objects that are
// registered by DictionaryEntry::add_protection_domain() to a unique entry. The entry
// is a WeakHandle that holds the protection domain oop.

or points to.... either is accurate.

It is the "maps ... to a unique entry" that I find most problematic - it begs the question as to what the unique entry is, when in reality it maps a PD instance (wrapped in a WeakHandle) to itself (wrapped in a WeakHandle).

For the sake of progress, approved.

The "unique" means there's only one value in the table for this protection domain oop. So the oop is unique. The use of word "mapping" might be what's confusing.
If we ever have to visit this code again (which I hope not, except to remove it because the security manager is finally removed since not many use it), we can see if this comment still makes sense to at least some of us.
Thanks for approving for progress.

src/hotspot/share/classfile/protectionDomainCache.cpp

iklam · 2022-09-01T07:12:25Z

I am utterly confused about why we need ProtectionDomainCacheTable at all. The only interface between this class and the rest of the world is:

DictionaryEntry::add_protection_domain() {
 
    WeakHandle obj = ProtectionDomainCacheTable::add_if_absent(protection_domain);
    // Additions and deletions hold the SystemDictionary_lock, readers are lock-free
    ProtectionDomainEntry* new_head = new ProtectionDomainEntry(obj, _pd_set);
}

(and there's code elsewhere for cleaning up this table, but that wouldn't be necessary if no one calls add_if_absent!).

Why doesn't DictionaryEntry::add_protection_domain allocate the WeakHandle itself?

I am looking at the JDK 8 code. It seems like ProtectionDomainCacheTable was needed before we had WeakHandle, so we had to do all the reference management by hand:

https://hg.openjdk.java.net/jdk8/jdk8/hotspot/file/87ee5ee27509/src/share/vm/classfile/dictionary.hpp#l137

But for today, is ProtectionDomainCacheTable a relic that can be thrown away?

dholmes-ora · 2022-09-01T07:54:55Z

I am utterly confused about why we need ProtectionDomainCacheTable at all.

It tracks whether we have seen this PD before so that validate_protection_domain can skip the Java upcall to checkPackageAccess.

coleenp · 2022-09-01T12:17:47Z

The reason we have the table is that you can have the same instance of java.security.ProtectionDomain used on multiple DictionaryEntry._pd_set lists. That is, multiple classes in the class loader data have had their loading initiated with the same protection domain, which is common. Creating WeakHandles for each is redundant and wasteful, so we want to have one WeakHandle for multiple entries to point to. The table is a place to store it. I had another version that stored the WeakHandle entries in the ClassLoaderData of the Dictionary of loaded classes that had them in their pd_set, but they had a linear lookup to determine if they were unique. We even performance tested that. In the end, I just translated the table to ResourceHashtable to make it easier to review and understand what this does.

As an example, if you run hello world with the security manager allowed, you see this:

java -Xlog:protectiondomain=trace -Djava.security.manager=allow -cp ~/work Hello
[0.188s][trace][protectiondomain] adding protection domain for class java/lang/Object class loader: a 'jdk/internal/loader/ClassLoaders$AppClassLoader'{0x00000007fef58f90} protection domain: a 'java/security/ProtectionDomain'{0x000000011f01d820} pd set count = #1
[0.189s][trace][protectiondomain] adding protection domain for class java/lang/String class loader: a 'jdk/internal/loader/ClassLoaders$AppClassLoader'{0x00000007fef58f90} protection domain: a 'java/security/ProtectionDomain'{0x000000011f01d820} pd set count = #1
[0.189s][trace][protectiondomain] adding protection domain for class java/lang/Exception class loader: a 'jdk/internal/loader/ClassLoaders$AppClassLoader'{0x00000007fef58f90} protection domain: a 'java/security/ProtectionDomain'{0x000000011f01d820} pd set count = #1
[0.189s][trace][protectiondomain] adding protection domain for class java/lang/System class loader: a 'jdk/internal/loader/ClassLoaders$AppClassLoader'{0x00000007fef58f90} protection domain: a 'java/security/ProtectionDomain'{0x000000011f01d820} pd set count = #1
[0.189s][trace][protectiondomain] adding protection domain for class java/io/PrintStream class loader: a 'jdk/internal/loader/ClassLoaders$AppClassLoader'{0x00000007fef58f90} protection domain: a 'java/security/ProtectionDomain'{0x000000011f01d820} pd set count = #1
Hello5

Whenever we initiate loading (for example) java.lang.Object in a non bootstrap class loader with the SecureClassLoader, we pass this protection domain. Notice how the same protection domain is added to each class. Then if we lookup java.lang.Object again on this loader with this protection domain, it's quickly found.

In JDK8 we pointed to the oop itself which required the GC to walk the global SystemDictionary. We definitely never want that, so moved it to a table, and made several other changes to enable concurrent class unloading after that - moved the Dictionaries of loaded classes to each class loader data.

coleenp · 2022-09-02T13:06:55Z

Thank you David and Ioi for approving and reviewing.
/integrate

openjdk · 2022-09-02T13:07:56Z

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

6fc58b8: 8293207: Add assert to JVM_ReferenceRefersTo to clarify its API
ce06a3b: 8293023: Change CardTable::is_in_young signature
26cac08: 8293209: Parallel: Remove unused variables in PSParallelCompact::invoke
46523b8: 8293219: Microsoft toolchain selection picks 32-bit tools over 64-bit
bc5ffc8: 8293100: RISC-V: Need to save and restore callee-saved FloatRegisters in StubGenerator::generate_call_stub
98ce45f: 8292981: Unify and restructure integer printing format specifiers
3ac91b0: 8293003: Review running time of Warn5 regression test
e0168a0: 8288012: AArch64: unnecessary macro expansion in stubGenerator_aarch64
99c3ab0: 8293010: JDI ObjectReference/referringObjects/referringObjects001 fails: assert(env->is_enabled(JVMTI_EVENT_OBJECT_FREE)) failed: checking
0fb9469: 8290126: Add a check in JavadocTester for "javadoc should not crash"
... and 81 more: https://git.openjdk.org/jdk/compare/95a33fe1502b6f0db2c60fa92b389fda74d94407...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-02T13:08:10Z

@coleenp Pushed as commit fcc0cf9.

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

coleenp added 2 commits August 25, 2022 20:51

8292375: Convert ProtectionDomainCacheTable to ResourceHashtable

40d4397

Beef up the Test for statistics

20eeab3

openjdk bot added the rfr Pull request is ready for review label Aug 26, 2022

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Aug 26, 2022

coleenp added 2 commits August 26, 2022 14:09

Cleanup things I just noticed.

1a81753

include mutexLocker.hpp for minimal build.

842b30a

dholmes-ora reviewed Aug 29, 2022

View reviewed changes

coleenp added 2 commits August 29, 2022 12:21

David's comments

ac67b18

Add comment about key,value.

b549a47

iklam approved these changes Aug 30, 2022

View reviewed changes

src/hotspot/share/classfile/protectionDomainCache.cpp Outdated Show resolved Hide resolved

src/hotspot/share/classfile/protectionDomainCache.hpp Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label Aug 30, 2022

coleenp commented Aug 30, 2022

View reviewed changes

src/hotspot/share/classfile/protectionDomainCache.cpp Outdated Show resolved Hide resolved

src/hotspot/share/classfile/protectionDomainCache.hpp Show resolved Hide resolved

coleenp added 2 commits August 30, 2022 20:12

Ioi comments

3634521

Fix comments, add assert.

8bdc90e

dholmes-ora reviewed Sep 1, 2022

View reviewed changes

src/hotspot/share/classfile/protectionDomainCache.cpp Show resolved Hide resolved

dholmes-ora approved these changes Sep 2, 2022

View reviewed changes

iklam approved these changes Sep 2, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Sep 2, 2022

openjdk bot closed this Sep 2, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 2, 2022

coleenp deleted the pd-nochange branch September 2, 2022 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8292375: Convert ProtectionDomainCacheTable to ResourceHashtable #10043

8292375: Convert ProtectionDomainCacheTable to ResourceHashtable #10043

coleenp commented Aug 26, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 26, 2022

openjdk bot commented Aug 26, 2022

mlbridge bot commented Aug 26, 2022 •

edited

Loading

dholmes-ora left a comment

dholmes-ora Aug 29, 2022 •

edited

Loading

coleenp Aug 29, 2022 •

edited

Loading

coleenp commented Aug 29, 2022 •

edited

Loading

iklam left a comment

openjdk bot commented Aug 30, 2022 •

edited

Loading

coleenp left a comment

dholmes-ora Sep 1, 2022

iklam Sep 1, 2022

dholmes-ora Sep 1, 2022

iklam Sep 1, 2022

dholmes-ora Sep 1, 2022

coleenp Sep 1, 2022

coleenp Sep 1, 2022

dholmes-ora Sep 2, 2022

coleenp Sep 2, 2022

iklam commented Sep 1, 2022

dholmes-ora commented Sep 1, 2022

coleenp commented Sep 1, 2022 •

edited

Loading

coleenp commented Sep 2, 2022

openjdk bot commented Sep 2, 2022

openjdk bot commented Sep 2, 2022

		// The ProtectionDomainCacheTable maps all java.security.ProtectionDomain objects that are
		// registered by DictionaryEntry::add_protection_domain() to a unique WeakHandle.

8292375: Convert ProtectionDomainCacheTable to ResourceHashtable #10043

8292375: Convert ProtectionDomainCacheTable to ResourceHashtable #10043

Conversation

coleenp commented Aug 26, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Aug 26, 2022

openjdk bot commented Aug 26, 2022

mlbridge bot commented Aug 26, 2022 • edited Loading

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

coleenp Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

coleenp commented Aug 29, 2022 • edited Loading

iklam left a comment

Choose a reason for hiding this comment

openjdk bot commented Aug 30, 2022 • edited Loading

coleenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iklam commented Sep 1, 2022

dholmes-ora commented Sep 1, 2022

coleenp commented Sep 1, 2022 • edited Loading

coleenp commented Sep 2, 2022

openjdk bot commented Sep 2, 2022

openjdk bot commented Sep 2, 2022

coleenp commented Aug 26, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 26, 2022 •

edited

Loading

dholmes-ora Aug 29, 2022 •

edited

Loading

coleenp Aug 29, 2022 •

edited

Loading

coleenp commented Aug 29, 2022 •

edited

Loading

openjdk bot commented Aug 30, 2022 •

edited

Loading

coleenp commented Sep 1, 2022 •

edited

Loading