Bug ID: JDK-8195744 Avoid calling ClassLoader.checkPackageAccess if security manager is not installed

Type: Enhancement
Component: hotspot
Sub-Component: runtime
Affected Version: 10

Priority: P3
Status: Resolved
Resolution: Fixed

Submitted: 2018-01-19
Updated: 2021-08-18
Resolved: 2021-02-08

JDK 17
17 b09Fixed

During class resolution, the JVM calls ClassLoader.checkPackageAccess(), and then updates DictionaryEntry::_pd_set accordingly.

See 
http://hg.openjdk.java.net/jdk/hs/file/5264a11d3753/src/hotspot/share/classfile/systemDictionary.cpp#l438

    JavaCalls::call_special(&result,
                         class_loader,
                         system_loader,
                         vmSymbols::checkPackageAccess_name(),
                         ...);
    ....
    if (HAS_PENDING_EXCEPTION) return;
    ....
    dictionary->add_protection_domain(d_index, d_hash, klass,
                                      protection_domain, THREAD);

However, when the security manager is not installed, ClassLoader.checkPackageAccess() does nothing:

http://hg.openjdk.java.net/jdk/hs/file/5264a11d3753/src/java.base/share/classes/java/lang/ClassLoader.java#l669

    private void checkPackageAccess(Class<?> cls, ProtectionDomain pd) {
        final SecurityManager sm = System.getSecurityManager();
        if (sm != null) {
            ... do some checks ....
        }
    }

So the JVM is making all these Java upcalls and maintaining a complicated cache for nothing. This causes slow down both in class loading time, as well as in GC pauses.

Preliminary benchmarking shows about 1.5% speed up when running hello world in clojure.

Changeset: ace8f946 Author: Coleen Phillimore <coleenp@openjdk.org> Date: 2021-02-08 21:31:25 +0000 URL: https://git.openjdk.java.net/jdk/commit/ace8f946
08-02-2021
According to [~iklam]'s benchmark test, I get a slight improvement with these changes (minus is good). 1. if !has_security_manager, elide calling checkPackageAccess Results of " perf stat -r 30 bin/java -Xshare:on -XX:SharedArchiveFile=zprint2.jsa -cp ./zprint-filter-fixed.jar ZPrintBench " instr delta = -49246898 -0.5332% time delta = -2.594 ms -0.2923% Results of " perf stat -r 10 bin/javac -J-Xlog:class+load=debug -J-Xshare:on -J-XX:SharedArchiveFile=javac2.jsa Bench_HelloWorld.java " instr delta = 1187141 0.0437% time delta = 0.719 ms 0.1415% 2. using -Djava.security.manager=disallow Results of " perf stat -r 30 bin/java -Xshare:on -Djava.security.manager=disallow -XX:SharedArchiveFile=zprint2.jsa -cp ./zprin t-filter-fixed.jar ZPrintBench " instr delta = -45072131 -0.4892% time delta = -4.736 ms -0.5350% Results of " perf stat -r 10 bin/javac -J-Djava.security.manager=disallow -J-Xlog:class+load=debug -J-Xshare:on -J-XX:SharedArchiveFile=javac2.jsa Bench_HelloWorld.java " instr delta = 1187141 0.0437% time delta = -8.649 ms -1.6954%
04-02-2021
My patch didn't call checkPackageAccess but it also didn't record the protection_domain for the loaded class, then when the class was loaded again after the security manager was installed, the previous class wasn't found and checkPackageAccess was called again which failed. It should have returned the previously loaded class.
03-02-2021
Sorry I missed that "disallow" option when reading the source code doc comment. I don't understand what you mean by it doesn't work. Currently we call ClassLoader.checkPackageAccess which checks if System.getSecurityManager() is null and if so it does nothing. The proposal is that the VM "calls" System.getSecurityManager() and if it is null it doesn't call checkPackageAccess. Whether the check is in the VM or in Java should make no difference as the logic is identical. Both approaches should succeed or fail in the same way. ??
03-02-2021
Unfortunately, that's the first thing I tried, but it doesn't work because you can change the security manager from one to another and the packageAccess seems to be recorded on the security manager (so tests fail). The option -Djava.security.manger=disallow was added in the bug report that Mandy pointed out.
03-02-2021
The default is no security manager already. "disallow" is not an option. As Mandy points out we still need a dynamic check for the security manager as it can be installed at any time, but we can do that by peeking into java.lang.System directly from the VM, which is cheap.
02-02-2021
This seems like a good optimization providing that enough applications supply this property to benefit from the optimization: -Djava.security.manager=disallow Or is can this become the default ?
02-02-2021
The security manager may be installed at runtime via System::setSecurityManager. JDK-8191053 proposes to provide a mechanism to flag that security manager is immutable in this execution. This way will enable reliable optimization when the VM can call java_lang_System::has_security_manager() to determine the security manager is null and skip the upcall to ClassLoader::checkPackageAccess.
23-01-2018
I really want to get rid of the pd cache altogether (or move it into Java code). However, that may have a performance impact. See http://hg.openjdk.java.net/jdk/hs/file/5264a11d3753/src/hotspot/share/classfile/dictionary.cpp#l366 InstanceKlass* Dictionary::find(unsigned int hash, Symbol* name, Handle protection_domain) { NoSafepointVerifier nsv; int index = hash_to_index(hash); DictionaryEntry* entry = get_entry(index, hash, name); if (entry != NULL && entry->is_valid_protection_domain(protection_domain)) { return entry->instance_klass(); } else { return NULL; } } If we get rid of the cache, when a security manager is installed, we would have to make an upcall to ClassLoader.checkPackageAccess for each call to Dictionary::find(). Doing that may significantly slowed down class resolution.
19-01-2018

Relates :	JDK-8272608 - java_lang_System::allow_security_manager() doesn't set its initialization flag
Relates :	JDK-8195665 - Rename DictionaryEntry::_pd_set to avoid ambiguity
Relates :	JDK-8191053 - Provide a mechanism to make system's security manager immutable