RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged
dean.long at oracle.com
dean.long at oracle.com
Thu Nov 1 03:59:12 UTC 2018
On 10/31/18 6:01 PM, Ioi Lam wrote:
> On 10/31/18 5:13 PM, dean.long at oracle.com wrote:
>> On 10/31/18 4:06 PM, David Holmes wrote:
>>> Hi Dean,
>>> Looking only at the hotspot changes. The removal of the DoPrivileged
>>> and related privileged_stack code seems okay. I have a few related
>>> You added the java_security_AccessController class after
>>> java_security_AccessControlContext. Did you actually verify where in
>>> the load/initialization order the AccessController class appears
>>> today, and where it appears after your change? (Note the comments at
>>> the start of WK_KLASSES_DO). Changes to the initialization order
>>> would be my biggest concern with this patch.
>> No, I did not notice that comment and assumed alphabetical order was
>> OK here. However, these classes appear to be only resolved, not
>> initialized (and AccessController does not have a static
>> initializer), so could you explain how the order in this list can
>> affect initialization order?
>> I only need this in JVM_GetStackAccessControlContext, which is a
>> static JNI method, so I could get the klass* from the incoming jclass
>> instead of using a well-known class entry.
> Hi Dean,
> You're correct that those classes are only resolved, and not
> initialized. So the order shouldn't matter too much. However, the
> order of the first few classes is important, as the creation of
> Object, Serializable, Cloneable, String, etc, has to be done in a
> certain order.
> I'm not sure whether the order of the reference classes, 292 classes,
> and box classes are important. I'll experiment of getting rid of the
> separate phases after the call to Universe::fixup_mirrors(). This
> might be relics from old days where the classes were once indeed
> initialized in SystemDictionary::initialize_well_known_classes, which
> was the old name for SystemDictionary::resolve_well_known_classes.
> (-XX:+TraceBytecodes shows no Java code is executed before
> resolve_well_known_classes returns).
> I filed https://bugs.openjdk.java.net/browse/JDK-8213230
> For the time being, I think as long as you put the new
> AccessController class near the existing class AccessControlContext,
> you should be OK.
> - Ioi
>>> I'm unclear about the change to the test:
>>> as it still refers to the now non-existent JVM_DoPrivileged in the
>>> summary. Does this test still make sense? Should it be moved to the
>>> Java side now it doesn't actually test anything in the VM?
>> I think these tests still make sense, unless we have similar coverage
>> somewhere else. How about if I fix the reference to JVM_DoPrivileged
>> for now and file a separate bug about moving or removing these tests?
>>> There may be further dead code to remove now:
>>> vmSymbols.hpp: codesource_permissioncollection_signature is now
>>> unreferenced and can be removed.
>>> - java_lang_System::has_security_manager
>>> - java_security_AccessControlContext::is_authorized
>>> ./share/memory/universe.hpp: static Method*
>> Good catches! I have uploaded a new webrev:
>> http://cr.openjdk.java.net/~dlong/8212605/webrev.2.incr/ (incremental
>>> On 1/11/2018 8:23 AM, dean.long at oracle.com wrote:
>>>> This change implements AccessController.doPrivileged in Java. This
>>>> gives a performance improvement while also being useful to Project
>>>> Loom by removing the Java --> native --> Java transition. One
>>>> reason doPrivileged has historically been in native is because of
>>>> the need to guarantee the cleanup of the privileged context when
>>>> doPrivileged returns. To do that in Java, the information that
>>>> AccessController.getContext needs is pushed onto the Java stack as
>>>> arguments to a method that getContext will recognize during its
>>>> stack walk. This allows us to remove the native privileged stack
>>>> while guaranteeing that the privileged context goes away when the
>>>> method returns.
>>>> Tested with tier1-tier3 hotspot and jdk tests and JCK
>>>> api/java_security tests. For the first few rounds of testing, I
>>>> kept the old native privileged stack and compared the results of
>>>> the old and new implementations for each getContext call, which did
>>>> catch some early bugs. The changes were also examined by internal
>>>> security experts and run through additional internal security tests.
>>>> The improvement on this  doPrivileged microbenchmark is
>>>> approximate 50x.
>>>> There is no attempt to optimize getContext() or security permission
>>>> checks in this change, however, this is intended to be a first step
>>>> towards other possible improvements, for example those proposed
>>>> here .
More information about the hotspot-dev