Hi Martin,
No further comments from me. I'm obviously not knowledgeable enough to
review any of the assembler changes.
Thanks,
David
On 23/07/2019 8:29 pm, Doerr, Martin wrote:
Hi David and Erik,
thank you for reviewing and for your very valuable feedback.
1) In the x86_64 assembly, you can combine the movl; testl; into a single
test instruction with one memory operand to the counter, and one
immediate zero.
Thanks for the hint. I'm using cmp32 in my new webrev.
2) If libjvm.so maps in far away, then the movl taking an ExternalAddress,
will actually scratch rscratch1, which is r10.
Good catch! I've exchanged registers and added assert_different_registers.
I was secretly hoping to never have to touch fast JNI getfield again,
because it is so shady, and the odd cases are very hard to test, making it so
easy to mess up. The ForceUnreachable JVM flag might be useful in checking
if a solution works also when rscratch1 gets clobbered when referencing JVM
symbols that are now "far away".
I've also changed the test to run with -XX:+ForceUnreachable and
-XX:+SafepointALot to hit more corner cases.
But as you explained, the test would normally not notice the destroyed counter
and just execute the slow path.
The subtle issue of referencing JVM symbols that can be far away,
suddenly clobbering r10, has bitten us many times. Perhaps it should be
made more explicit somehow.
It would be possible to explicitly kill r10 in all such assembler instructions
in the dbg build, but that'd come with an overhead.
But that's a separate issue.
Agreed.
Also, I noticed that the counter that we are checking if it has changed, is a
32 bit signed integer. They can actually wrap around, which is undefined
behaviour at best, and will make these tests fail in the worst case. When we
don't want counters to overflow, we use 64 bit integers.
We could also make it unsigned to get defined behavior, but that's out of scope
here.
New webrev:
http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.01/
Best regards,
Martin
