RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

Thu Nov 1 17:15:30 UTC 2018

Hi Peter.  Thanks for the input.  I don't know about DomainCombiner, but 
perhaps someone else on this list does.

dl

On 10/31/18 6:15 PM, Peter wrote:
> Hello Dean & David,
>
> Interesting reading.   Is DomainCombiner still being considered for 
> deprecation?
>
> Our code makes heavy use of AccessController.doPrivileged.
>
> Regards,
>
> Peter.
>
> "Hot Spots - Method","Self time [%]","Self time","Self time 
> (CPU)","Samples"
> "java.net.DualStackPlainSocketImpl.accept0[native]()","38.88627","1837851.568 
> ms","1837851.568 ms","16"
> "java.net.SocketInputStream.socketRead0[native]()","20.505322","969127.11 
> ms","969127.11 ms","315"
> "java.net.TwoStacksPlainDatagramSocketImpl.peekData[native]()","20.19654","954533.467 
> ms","954533.467 ms","11"
> "sun.management.ThreadImpl.dumpThreads0[native]()","11.148865","526920.162 
> ms","526920.162 ms","239"
> "java.net.TwoStacksPlainDatagramSocketImpl.socketNativeSetOption[native]()","5.3582244","253241.609 
> ms","253241.609 ms","17"
> "sun.misc.Unsafe.unpark[native]()","2.6599514","125715.216 
> ms","125715.216 ms","759"
> "sun.misc.Unsafe.park[native]()","0.19931908","1.6213131447E7 
> ms","9420.263 ms","1401"
> "java.util.concurrent.ScheduledThreadPoolExecutor$DelayedWorkQueue.siftDown()","0.15738875","7438.542 
> ms","7438.542 ms","71"
> "java.security.AccessController.doPrivileged[native]()","0.10248028","4843.446 
> ms","4843.446 ms","715"
> "java.lang.Thread.isInterrupted[native]()","0.09570652","4523.303 
> ms","4523.303 ms","40"
> "java.net.NetworkInterface.getAll[native]()","0.07478044","3534.29 
> ms","3534.29 ms","4"
> "java.util.concurrent.ThreadPoolExecutor.runWorker()","0.046051156","2176.48 
> ms","2176.48 ms","91"
> "java.lang.SecurityManager.getClassContext[native]()","0.043098312","2036.922 
> ms","2036.922 ms","20"
> "java.security.AccessController.getStackAccessControlContext[native]()","0.039155032","1850.554 
> ms","1850.554 ms","15"
> "au.net.zeus.collection.ReferenceProcessor$EnqueGarbageTask.run()","0.035316195","1669.122 
> ms","1669.122 ms","18"
> "java.net.SocketOutputStream.socketWrite0[native]()","0.03470353","1640.166 
> ms","1640.166 ms","17"
> "java.lang.Class.forName0[native]()","0.029112596","1375.926 
> ms","1375.926 ms","10"
> "java.lang.Throwable.fillInStackTrace[native]()","0.028279837","1336.568 
> ms","1336.568 ms","18"
> "java.lang.Thread.holdsLock[native]()","0.023839759","1126.72 
> ms","1126.72 ms","7"
> "java.lang.String.indexOf()","0.019056467","900.651 ms","900.651 ms","1"
> "au.net.zeus.collection.AbstractReferenceComparator.compare()","0.017307268","817.98 
> ms","817.98 ms","18"
> "sun.reflect.Reflection.getCallerClass[native]()","0.017031383","804.941 
> ms","804.941 ms","11"
> "java.net.DualStackPlainSocketImpl.available0[native]()","0.015988858","755.669 
> ms","755.669 ms","14"
> "java.lang.String.intern[native]()","0.014759737","697.578 
> ms","697.578 ms","8"
> "au.net.zeus.collection.ReferenceMap.wrapKey()","0.0139064975","657.252 
> ms","657.252 ms","4"
> "org.apache.river.api.net.Uri.toAsciiLowerCase()","0.013079452","618.164 
> ms","618.164 ms","1"
> "sun.reflect.DelegatingMethodAccessorImpl.invoke()","0.012204483","576.811 
> ms","576.811 ms","753"
> "java.lang.Object.clone[native]()","0.011227106","530.618 ms","530.618 
> ms","4"
> "java.lang.Class.getClassLoader0[native]()","0.010788701","509.898 
> ms","509.898 ms","9"
> "org.apache.river.impl.thread.SynchronousExecutors$Distributor.run()","0.010243974","484.153 
> ms","484.153 ms","3"
> "java.util.concurrent.locks.AbstractQueuedSynchronizer.release()","0.008626911","407.727 
> ms","407.727 ms","4"
> "java.util.concurrent.ConcurrentSkipListMap.keyIterator()","0.008402821","397.136 
> ms","397.136 ms","4"
> "sun.reflect.misc.ReflectUtil.checkPackageAccess()","0.008291823","391.89 
> ms","391.89 ms","8"
> "java.util.concurrent.ScheduledThreadPoolExecutor.schedule()","0.0078934925","373.064 
> ms","373.064 ms","1"
> "sun.management.VMManagementImpl.getDaemonThreadCount[native]()","0.007857989","371.386 
> ms","371.386 ms","1"
> "java.util.concurrent.ScheduledThreadPoolExecutor$DelayedWorkQueue.take()","0.0068998444","326.102 
> ms","326.102 ms","255"
> "java.lang.Class.getConstantPool[native]()","0.0065752934","310.763 
> ms","310.763 ms","2"
> "net.jini.jeri.connection.ConnectionManager.connect()","0.0060991626","8072.071 
> ms","288.26 ms","14"
> "java.util.concurrent.Executors$RunnableAdapter.call()","0.006036745","285.31 
> ms","285.31 ms","797"
> "java.util.Collections$SynchronizedCollection.size()","0.0057329717","270.953 
> ms","270.953 ms","1"
> "sun.management.ThreadImpl.getThreadInfo1[native]()","0.005478752","258.938 
> ms","258.938 ms","4"
> "java.util.concurrent.ScheduledThreadPoolExecutor.reExecutePeriodic()","0.00411447","194.459 
> ms","194.459 ms","3"
> "java.lang.reflect.Array.newArray[native]()","0.003456418","163.358 
> ms","163.358 ms","1"
> "com.sun.jini.jeri.internal.mux.MuxInputStream.read()","0.003161764","149.432 
> ms","149.432 ms","17"
> "net.jini.loader.pref.PreferredClassProvider.lookupLoader()","0.0031255828","147.722 
> ms","147.722 ms","2"
> "java.lang.Class.isPrimitive[native]()","0.0030873707","145.916 
> ms","145.916 ms","2"
> "au.net.zeus.collection.ReferenceMap.get()","0.0029420536","139.048 
> ms","139.048 ms","6"
> "sun.reflect.MethodAccessorGenerator.buildInternalSignature()","0.0029316437","138.556 
> ms","138.556 ms","1"
> "sun.management.ThreadImpl.findDeadlockedThreads0[native]()","0.002746041","129.784 
> ms","129.784 ms","2"
> "org.apache.river.api.io.AtomicObjectInputStream.readClassDesc()","0.0026871779","127.002 
> ms","127.002 ms","178"
> "net.jini.security.GrantPermission.constructName()","0.0026251199","124.069 
> ms","124.069 ms","1"
> "java.lang.System.identityHashCode[native]()","0.002610605","123.383 
> ms","123.383 ms","2"
> "java.util.concurrent.locks.LockSupport.unpark()","0.0025506418","120.549 
> ms","120.549 ms","760"
> "java.util.concurrent.locks.ReentrantReadWriteLock$NonfairSync.readerShouldBlock()","0.0025335667","119.742 
> ms","119.742 ms","2"
> "java.util.concurrent.ConcurrentSkipListMap.findPredecessor()","0.0024627491","116.395 
> ms","116.395 ms","4"
> "java.lang.SecurityManager.checkPackageAccess()","0.0023379137","110.495 
> ms","110.495 ms","1"
> "java.lang.Class.checkPackageAccess()","0.0022838535","107.94 
> ms","107.94 ms","5"
> "net.jini.loader.LoadClass.forName()","0.002258442","106.739 
> ms","106.739 ms","163"
> "java.io.ObjectOutputStream$HandleTable.growSpine()","0.0022080212","104.356 
> ms","104.356 ms","1"
> "java.lang.AbstractStringBuilder.deleteCharAt()","0.0021902905","103.518 
> ms","103.518 ms","1"
> "javax.management.openmbean.CompositeDataSupport.<init>()","0.0021849375","103.265 
> ms","103.265 ms","2"
> "java.io.ObjectStreamClass$FieldReflector.getObjFieldValues()","0.0021076875","99.614 
> ms","99.614 ms","1"
> "java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject.addConditionWaiter()","0.0021062698","99.547 
> ms","99.547 ms","1"
> "sun.management.ThreadImpl.getThreadInfo()","0.002029676","95.927 
> ms","95.927 ms","5"
> "au.net.zeus.collection.AbstractReferrerDecorator.get()","0.0019218308","90.83 
> ms","90.83 ms","1"
> "org.apache.river.api.io.AtomicObjectInputStream$GetArgImpl.get()","0.0019038884","89.982 
> ms","89.982 ms","24"
> "java.util.concurrent.ConcurrentSkipListMap$Node.appendMarker()","0.0019032748","89.953 
> ms","89.953 ms","1"
> "java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire()","0.001819889","86.012 
> ms","86.012 ms","6"
> "java.security.Policy.getPolicy()","0.0017857603","84.399 ms","84.399 
> ms","1"
> "java.net.URL.toExternalForm()","0.0017011049","80.398 ms","80.398 
> ms","2"
> "java.io.ObjectOutputStream$HandleTable.lookup()","0.0016315778","77.112 
> ms","77.112 ms","2"
> "java.lang.Class.getSuperclass[native]()","0.0015686523","74.138 
> ms","74.138 ms","1"
> "net.jini.loader.pref.PreferredClassLoader.checkPermissions()","0.0015065097","71.201 
> ms","71.201 ms","14"
> "org.apache.river.api.security.PermissionComparator.compare()","0.0014423993","68.171 
> ms","68.171 ms","2"
> "org.apache.river.api.io.AtomicObjectInputStream.findStreamSuperclass()","0.0013877256","65.587 
> ms","65.587 ms","1"
> "java.security.AccessController.getContext()","0.0013715605","64.823 
> ms","64.823 ms","17"
> "java.lang.Boolean.equals()","0.0013067942","61.762 ms","61.762 ms","2"
> "java.lang.String.valueOf()","0.0012928719","61.104 ms","61.104 ms","6"
> "java.io.ObjectStreamField.<init>()","0.001281023","60.544 ms","60.544 
> ms","4"
> "org.apache.river.api.io.AtomicObjectInputStream.getBaseName()","0.0011021064","52.088 
> ms","52.088 ms","1"
>
> On 1/11/2018 10:54 AM, David Holmes wrote:
>> Hi Dean,
>>
>> On 1/11/2018 10:13 AM, 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 comments:
>>>>
>>>> src/hotspot/share/classfile/systemDictionary.hpp
>>>>
>>>> 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?
>>
>> You're right it doesn't. There are a couple of comments that refer to 
>> "initialization" but it's not static initialization of these classes.
>>
>> I'm unclear how the resolution order in that list may interact with 
>> other parts of the startup sequence.
>>
>>> 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.
>>
>> I think it's okay given we have AccessControlContext there anyway.
>>
>>>> ---
>>>>
>>>> I'm unclear about the change to the test:
>>>>
>>>> test/hotspot/jtreg/runtime/JVMDoPrivileged/DoPrivRunAbstract.jasm
>>>>
>>>> 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?
>>
>> Yep that's fine.
>>
>>>> ---
>>>>
>>>> There may be further dead code to remove now:
>>>>
>>>> vmSymbols.hpp: codesource_permissioncollection_signature is now 
>>>> unreferenced and can be removed.
>>>>
>>>> javaClasses.*:
>>>>   - java_lang_System::has_security_manager
>>>>   - java_security_AccessControlContext::is_authorized
>>>>
>>>> ./share/memory/universe.hpp:  static Method* 
>>>> protection_domain_implies_method()
>>>>
>>>
>>> Good catches!  I have uploaded a new webrev:
>>>
>>> http://cr.openjdk.java.net/~dlong/8212605/webrev.2/
>>> http://cr.openjdk.java.net/~dlong/8212605/webrev.2.incr/ 
>>> (incremental diff)
>>
>> Updates all look fine.
>>
>> Thanks,
>> David
>> -----
>>
>>> dl
>>>
>>>> ---
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>
>>>> On 1/11/2018 8:23 AM, dean.long at oracle.com wrote:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8212605
>>>>> http://cr.openjdk.java.net/~dlong/8212605/webrev.1
>>>>>
>>>>> 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 [1] 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 [2].
>>>>>
>>>>> dl
>>>>>
>>>>> [1] 
>>>>> http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java 
>>>>>
>>>>> [2] 
>>>>> http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016627.html 
>>>>>
>>>>>
>>>
>