David, can I list you as a reviewer?
dl
On 12/16/18 8:47 PM, dean.l...@oracle.com wrote:
On 12/16/18 7:39 PM, dean.l...@oracle.com wrote:
On 12/16/18 7:03 PM, David Holmes wrote:
On 17/12/2018 12:49 pm, dean.l...@oracle.com wrote:
On 12/16/18 4:06 PM, David Holmes wrote:
On 15/12/2018 10:59 am, dean.l...@oracle.com wrote:
https://bugs.openjdk.java.net/browse/JDK-8214583
http://cr.openjdk.java.net/~dlong/8214583/webrev
This change includes two new regression test that demonstrate the
problem, and a fix that allows the tests
to pass.
The problem happens when the JIT compiler's escape analysis
eliminates the allocation of the AccessControlContext object
passed to doPrivileged. The compiler thinks this is safe because
it does not see that the object "escapes".
Then surely the compiler's notion of "escapes" needs to be updated!
The compiler can inline the callee method and see that the value
doesn't escape. This is a valid optimization in cases where the
callee method is known.
But it's not a valid optimization in this case, so my comment stands.
Is this stack walking something this is guaranteed by the spec to be
always valid (and hence the JIT is violating the rules), or is the
stack walking code making assumptions about whether it will find the
context object in the stack?
The stack walking is in the VM and is an internal implementation
detail, not part of the AccessController API spec. A different
thread running normal Java code would never be able to see a
non-escaping value. The stack walking code does need to find the
context object in the stack. Non-escaping objects won't show up in
the stack.
If we have to hack around this with an annotation I'd rather see a
specific annotation that addresses the problematic usecase than a
generic "don't inline" one. E.g. @StackVisible or something like that.
That sounds like a good idea for 13, but would require changes to
both C2 and Graal, and it seems a little risky compared to using
existing mechanisms.
I forgot to address this in my last reply, but I'm not suggesting a
@DontInline annotation. That was Claes. My fixes uses a native method.
dl
dl
Cheers,
David
dl
David
-----
However, getContext needs to be able to find
the object using a stack walk, so we need a way to tell the
compiler that it does indeed escape. To do this we pass the value
to a native method that does nothing.
Microbenchmark results:
jdk12-b18:
Benchmark Mode Cnt Score Error Units
DoPrivileged.test avgt 25 255.626 ± 6.446 ns/op
DoPrivileged.testInline avgt 25 250.968 ± 4.975 ns/op
jdk12-b19:
Benchmark Mode Cnt Score Error Units
DoPrivileged.test avgt 25 5.689 ± 0.001 ns/op
DoPrivileged.testInline avgt 25 2.765 ± 0.001 ns/op
this fix:
Benchmark Mode Cnt Score Error Units
DoPrivileged.test avgt 25 5.020 ± 0.001 ns/op
DoPrivileged.testInline avgt 25 2.774 ± 0.025 ns/op
dl
