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
