Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 03/25/2015 09:13 AM, Andrew Haley wrote: On 24/03/15 23:40, Vladimir Kozlov wrote: The test failed when run it in JPRT with 32-bit fastdebug *Client* VM (-client) on linux-x86: java.lang.RuntimeException at MyByteBuffer.ck(HeapByteBufferTest.java:201) at MyByteBuffer.getLong(HeapByteBufferTest.java:211) at HeapByteBufferTest.step(HeapByteBufferTest.java:311) at HeapByteBufferTest.run(HeapByteBufferTest.java:347) at HeapByteBufferTest.main(HeapByteBufferTest.java:362) Could be intrinsic in C1 does not work correctly? Please, look. I certainly will. That is odd: there's no reason I can think of why this might happen, and I know that the test running on a server build runs C1 code for a while so it has been tested. I guess it must be a rare edge case. It's not what I expected, and maybe not what you expected either. My test case fails on 32-bit x86 before any of my patches have been applied. It turns out that the problem is due to the handling of floating-point NaNs on legacy 32-bit x86 systems. The template interpreter uses 80x87 floating point but the compilers use XMM registers. XMM is transparent to all floating-point data: you can load and store any bit pattern and it is not altered. The same is not true of 80x87: if the operand of an operation is a signaling NaN, the default action is to silently convert it to a quiet NaN. This means that interpreted and compiled code will do different things with signaling NaNs. While I'm not sure if this is a specification violation, it certainly is a breach of the Write Once, Run Anywhere principle, albeit a very unimportant one. I've altered the test code so that when generating random bit patterns it never generates a signaling NaN. This makes for a clean run on 32-bit x86 systems. http://cr.openjdk.java.net/~aph/unaligned.jdk.9/ http://cr.openjdk.java.net/~aph/unaligned.hotspot.9/ Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
Thank you, Andrew I will file a separate bug to address this issue - Interpreter and compiled code should produce the same result! And I will push your changes today. Regards, Vladimir On 3/31/15 7:33 AM, Andrew Haley wrote: On 03/25/2015 09:13 AM, Andrew Haley wrote: On 24/03/15 23:40, Vladimir Kozlov wrote: The test failed when run it in JPRT with 32-bit fastdebug *Client* VM (-client) on linux-x86: java.lang.RuntimeException at MyByteBuffer.ck(HeapByteBufferTest.java:201) at MyByteBuffer.getLong(HeapByteBufferTest.java:211) at HeapByteBufferTest.step(HeapByteBufferTest.java:311) at HeapByteBufferTest.run(HeapByteBufferTest.java:347) at HeapByteBufferTest.main(HeapByteBufferTest.java:362) Could be intrinsic in C1 does not work correctly? Please, look. I certainly will. That is odd: there's no reason I can think of why this might happen, and I know that the test running on a server build runs C1 code for a while so it has been tested. I guess it must be a rare edge case. It's not what I expected, and maybe not what you expected either. My test case fails on 32-bit x86 before any of my patches have been applied. It turns out that the problem is due to the handling of floating-point NaNs on legacy 32-bit x86 systems. The template interpreter uses 80x87 floating point but the compilers use XMM registers. XMM is transparent to all floating-point data: you can load and store any bit pattern and it is not altered. The same is not true of 80x87: if the operand of an operation is a signaling NaN, the default action is to silently convert it to a quiet NaN. This means that interpreted and compiled code will do different things with signaling NaNs. While I'm not sure if this is a specification violation, it certainly is a breach of the Write Once, Run Anywhere principle, albeit a very unimportant one. I've altered the test code so that when generating random bit patterns it never generates a signaling NaN. This makes for a clean run on 32-bit x86 systems. http://cr.openjdk.java.net/~aph/unaligned.jdk.9/ http://cr.openjdk.java.net/~aph/unaligned.hotspot.9/ Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
Looks like only 32-bit is affected. Both Server (C2) and Client (C1) VMs. 64-bit VM pass all combinations: Tiered, C2 only (-TieredCompilation), C1 only (-XX:+TieredCompilation -XX:TieredStopAtLevel=1) with and without intrinsics. 32-bit Client VM (C1) fails with both, -XX:-UseUnalignedAccesses and -XX:+UseUnalignedAccesses. $ bin/java -client -XX:+UnlockDiagnosticVMOptions -XX:-UseUnalignedAccesses HeapByteBufferTest Exception in thread main java.lang.RuntimeException at MyByteBuffer.ck(HeapByteBufferTest.java:201) at MyByteBuffer.getLong(HeapByteBufferTest.java:211) at HeapByteBufferTest.step(HeapByteBufferTest.java:311) at HeapByteBufferTest.run(HeapByteBufferTest.java:347) at HeapByteBufferTest.main(HeapByteBufferTest.java:362) $ bin/java -client -XX:+UnlockDiagnosticVMOptions -XX:+UseUnalignedAccesses HeapByteBufferTest Exception in thread main java.lang.RuntimeException at MyByteBuffer.ck(HeapByteBufferTest.java:201) at MyByteBuffer.getLong(HeapByteBufferTest.java:211) at HeapByteBufferTest.step(HeapByteBufferTest.java:311) at HeapByteBufferTest.run(HeapByteBufferTest.java:347) at HeapByteBufferTest.main(HeapByteBufferTest.java:362) 32-bit Server VM without tiered (-TieredCompilation) FAILS too in both cases! Note, without intrinsics it failed in different place. $ bin/java -server -XX:-TieredCompilation -XX:+UnlockDiagnosticVMOptions -XX:-UseUnalignedAccesses HeapByteBufferTest Exception in thread main java.lang.RuntimeException at MyByteBuffer.ck(HeapByteBufferTest.java:207) at MyByteBuffer.getDouble(HeapByteBufferTest.java:215) at HeapByteBufferTest.step(HeapByteBufferTest.java:329) at HeapByteBufferTest.run(HeapByteBufferTest.java:347) at HeapByteBufferTest.main(HeapByteBufferTest.java:362) $ bin/java -server -XX:-TieredCompilation -XX:+UnlockDiagnosticVMOptions -XX:+UseUnalignedAccesses HeapByteBufferTest Exception in thread main java.lang.RuntimeException at MyByteBuffer.ck(HeapByteBufferTest.java:201) at MyByteBuffer.getLong(HeapByteBufferTest.java:211) at HeapByteBufferTest.step(HeapByteBufferTest.java:311) at HeapByteBufferTest.run(HeapByteBufferTest.java:347) at HeapByteBufferTest.main(HeapByteBufferTest.java:362) For some reasons Server VM only pass when TieredCompilation is enabled. Looking on -XX:+PrintCompilation -XX:+PrintInlining output I see that UseUnalignedAccesses affects intrinsics as designed. Thanks, Vladimir On 3/25/15 2:13 AM, Andrew Haley wrote: On 24/03/15 23:40, Vladimir Kozlov wrote: The test failed when run it in JPRT with 32-bit fastdebug *Client* VM (-client) on linux-x86: java.lang.RuntimeException at MyByteBuffer.ck(HeapByteBufferTest.java:201) at MyByteBuffer.getLong(HeapByteBufferTest.java:211) at HeapByteBufferTest.step(HeapByteBufferTest.java:311) at HeapByteBufferTest.run(HeapByteBufferTest.java:347) at HeapByteBufferTest.main(HeapByteBufferTest.java:362) Could be intrinsic in C1 does not work correctly? Please, look. I certainly will. That is odd: there's no reason I can think of why this might happen, and I know that the test running on a server build runs C1 code for a while so it has been tested. I guess it must be a rare edge case. Still, I'm quite pleased that the test I wrote detected the failure. Do you know if this was running with +UseUnalignedAccesses? I'm not going to be able to analyse this for a few days. Expect a report (and hopefully a fix) next week. Thanks, Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
Andrew, The test failed when run it in JPRT with 32-bit fastdebug *Client* VM (-client) on linux-x86: java.lang.RuntimeException at MyByteBuffer.ck(HeapByteBufferTest.java:201) at MyByteBuffer.getLong(HeapByteBufferTest.java:211) at HeapByteBufferTest.step(HeapByteBufferTest.java:311) at HeapByteBufferTest.run(HeapByteBufferTest.java:347) at HeapByteBufferTest.main(HeapByteBufferTest.java:362) Could be intrinsic in C1 does not work correctly? Please, look. Thanks, Vladimir On 3/24/15 1:15 AM, Andrew Haley wrote: On 23/03/15 21:10, Vladimir Kozlov wrote: Hotspot make files changes? There's no need: I think we came to the conclusion that we wouldn't define BIG_ENDIAN. Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 25/03/2015 4:07 AM, Vladimir Kozlov wrote: Got it. I missed that you changed code in unsafe.cpp. Let me prepare closed changes and get it reviewed before we can push this. Copyright notices need updating too. And the copyright dates on the test look like they have been copied from another file - given this is a new test. Thanks, David Thanks, Vladimir On 3/24/15 1:15 AM, Andrew Haley wrote: On 23/03/15 21:10, Vladimir Kozlov wrote: Hotspot make files changes? There's no need: I think we came to the conclusion that we wouldn't define BIG_ENDIAN. Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
Got it. I missed that you changed code in unsafe.cpp. Let me prepare closed changes and get it reviewed before we can push this. Thanks, Vladimir On 3/24/15 1:15 AM, Andrew Haley wrote: On 23/03/15 21:10, Vladimir Kozlov wrote: Hotspot make files changes? There's no need: I think we came to the conclusion that we wouldn't define BIG_ENDIAN. Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 23/03/15 21:10, Vladimir Kozlov wrote: Hotspot make files changes? There's no need: I think we came to the conclusion that we wouldn't define BIG_ENDIAN. Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
Hotspot make files changes? Otherwise looks fine. Vladimir On 3/23/15 11:31 AM, Andrew Haley wrote: On 03/21/2015 04:32 PM, Vladimir Kozlov wrote: On 3/21/15 4:36 AM, Andrew Haley wrote: On 20/03/15 22:00, Vladimir Kozlov wrote: On 3/20/15 9:08 AM, Andrew Haley wrote: Webrev at http://cr.openjdk.java.net/~aph/unaligned.hotspot.7/ I don't see changes to VM's makefiles to set VM_BIG_ENDIAN. Or consider it is set if VM_LITTLE_ENDIAN is not set in prims/unsafe.cpp code. Darn, I already made that change once but missed it in this webrev. Sorry. I will wait new webrev. In globals.hpp the empty line after flag description should not have '\' at the end. OK. Otherwise it looks good. http://cr.openjdk.java.net/~aph/unaligned.jdk.7/ My only comment for jdk change is to use bigEndian name instead of BE in Unsafe.java. Someone from core libs should look on this. I would have used that, but I followed John Rose's suggestion. I don't mind making changes but having to change it back-and-forth really is too much. John have more authority than me :) so be it BE. Ok. Sorry for being impatient. http://cr.openjdk.java.net/~aph/unaligned.hotspot.8/ http://cr.openjdk.java.net/~aph/unaligned.jdk.7/ Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 03/21/2015 04:32 PM, Vladimir Kozlov wrote: On 3/21/15 4:36 AM, Andrew Haley wrote: On 20/03/15 22:00, Vladimir Kozlov wrote: On 3/20/15 9:08 AM, Andrew Haley wrote: Webrev at http://cr.openjdk.java.net/~aph/unaligned.hotspot.7/ I don't see changes to VM's makefiles to set VM_BIG_ENDIAN. Or consider it is set if VM_LITTLE_ENDIAN is not set in prims/unsafe.cpp code. Darn, I already made that change once but missed it in this webrev. Sorry. I will wait new webrev. In globals.hpp the empty line after flag description should not have '\' at the end. OK. Otherwise it looks good. http://cr.openjdk.java.net/~aph/unaligned.jdk.7/ My only comment for jdk change is to use bigEndian name instead of BE in Unsafe.java. Someone from core libs should look on this. I would have used that, but I followed John Rose's suggestion. I don't mind making changes but having to change it back-and-forth really is too much. John have more authority than me :) so be it BE. Ok. Sorry for being impatient. http://cr.openjdk.java.net/~aph/unaligned.hotspot.8/ http://cr.openjdk.java.net/~aph/unaligned.jdk.7/ Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
If you say so, John, I am okay with BE. Vladimir On 3/20/15 10:21 PM, John Rose wrote: I'm glad to see this coming along… The code, javadoc, and spec all look good. On Mar 20, 2015, at 3:00 PM, Vladimir Kozlov vladimir.koz...@oracle.com mailto:vladimir.koz...@oracle.com wrote: http://cr.openjdk.java.net/~aph/unaligned.jdk.7/ My only comment for jdk change is to use bigEndian name instead of BE in Unsafe.java. Someone from core libs should look on this. Here's my take on that bit (subject of course to correction from a core-libs eng.). BE (though short), as an upper-case acronym, is more correct than mixed-case bigEndian, according to the conventions suggested by JLS 6.1: Constant Names The names of constants in interface types should be, and final variables of class types may conventionally be, a sequence of one or more words, acronyms, or abbreviations, all uppercase, with components separated by underscore _ characters. Constant names should be descriptive and not unnecessarily abbreviated. Conventionally they may be any appropriate part of speech. Examples of names for constants include MIN_VALUE, MAX_VALUE, MIN_RADIX, and MAX_RADIX of the class Character. So I think it's OK as-is, especially since it is a private name. — John
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 3/21/15 4:36 AM, Andrew Haley wrote: On 20/03/15 22:00, Vladimir Kozlov wrote: On 3/20/15 9:08 AM, Andrew Haley wrote: Webrev at http://cr.openjdk.java.net/~aph/unaligned.hotspot.7/ I don't see changes to VM's makefiles to set VM_BIG_ENDIAN. Or consider it is set if VM_LITTLE_ENDIAN is not set in prims/unsafe.cpp code. Darn, I already made that change once but missed it in this webrev. Sorry. I will wait new webrev. In globals.hpp the empty line after flag description should not have '\' at the end. OK. Otherwise it looks good. http://cr.openjdk.java.net/~aph/unaligned.jdk.7/ My only comment for jdk change is to use bigEndian name instead of BE in Unsafe.java. Someone from core libs should look on this. I would have used that, but I followed John Rose's suggestion. I don't mind making changes but having to change it back-and-forth really is too much. John have more authority than me :) so be it BE. I've moved the test case into the hotspot repo The test case is diagnostic, not experimental Yes, is is. I was told to do that. I am fine with diagnostic type of flag. The sponsor from Oracle have to set the flag in our closed sources similar to open vm_verstion_* changes - but it is small change. Thanks, Vladimir Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 20/03/15 22:00, Vladimir Kozlov wrote: On 3/20/15 9:08 AM, Andrew Haley wrote: Webrev at http://cr.openjdk.java.net/~aph/unaligned.hotspot.7/ I don't see changes to VM's makefiles to set VM_BIG_ENDIAN. Or consider it is set if VM_LITTLE_ENDIAN is not set in prims/unsafe.cpp code. Darn, I already made that change once but missed it in this webrev. Sorry. In globals.hpp the empty line after flag description should not have '\' at the end. OK. Otherwise it looks good. http://cr.openjdk.java.net/~aph/unaligned.jdk.7/ My only comment for jdk change is to use bigEndian name instead of BE in Unsafe.java. Someone from core libs should look on this. I would have used that, but I followed John Rose's suggestion. I don't mind making changes but having to change it back-and-forth really is too much. I've moved the test case into the hotspot repo The test case is diagnostic, not experimental Yes, is is. I was told to do that. Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
I'm glad to see this coming along… The code, javadoc, and spec all look good. On Mar 20, 2015, at 3:00 PM, Vladimir Kozlov vladimir.koz...@oracle.com wrote: http://cr.openjdk.java.net/~aph/unaligned.jdk.7/ http://cr.openjdk.java.net/~aph/unaligned.jdk.7/ My only comment for jdk change is to use bigEndian name instead of BE in Unsafe.java. Someone from core libs should look on this. Here's my take on that bit (subject of course to correction from a core-libs eng.). BE (though short), as an upper-case acronym, is more correct than mixed-case bigEndian, according to the conventions suggested by JLS 6.1: Constant Names The names of constants in interface types should be, and final variables of class types may conventionally be, a sequence of one or more words, acronyms, or abbreviations, all uppercase, with components separated by underscore _ characters. Constant names should be descriptive and not unnecessarily abbreviated. Conventionally they may be any appropriate part of speech. Examples of names for constants include MIN_VALUE, MAX_VALUE, MIN_RADIX, and MAX_RADIX of the class Character. So I think it's OK as-is, especially since it is a private name. — John
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
Webrev at http://cr.openjdk.java.net/~aph/unaligned.hotspot.7/ http://cr.openjdk.java.net/~aph/unaligned.jdk.7/ I've moved the test case into the hotspot repo The test case is diagnostic, not experimental Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 3/20/15 9:08 AM, Andrew Haley wrote: Webrev at http://cr.openjdk.java.net/~aph/unaligned.hotspot.7/ I don't see changes to VM's makefiles to set VM_BIG_ENDIAN. Or consider it is set if VM_LITTLE_ENDIAN is not set in prims/unsafe.cpp code. In globals.hpp the empty line after flag description should not have '\' at the end. Otherwise it looks good. http://cr.openjdk.java.net/~aph/unaligned.jdk.7/ My only comment for jdk change is to use bigEndian name instead of BE in Unsafe.java. Someone from core libs should look on this. I've moved the test case into the hotspot repo The test case is diagnostic, not experimental Good. Thanks, Vladimir Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
People, please. I have been working on this for weeks. All I am waiting on now is a decision about where the test case should go. Can I have a decision so we can get this in? Thanks, Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
Hi Andrew, Given no further advice, my inclination is move it to hotspot tests as i suspect the scope is at least as good, if not better. I agree we should not be blocked on this, so it's up to you :-) If left as is, we can/should revisit. Paul. On Mar 19, 2015, at 10:03 AM, Andrew Haley a...@redhat.com wrote: People, please. I have been working on this for weeks. All I am waiting on now is a decision about where the test case should go. Can I have a decision so we can get this in?
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
New code did not change API of existing X-Buffer methods. It changed only how they are implementation. And already existing nio/Buffer tests should test these methods already. Regards, Vladimir On 3/19/15 9:33 AM, Alan Bateman wrote: On 19/03/2015 15:59, Vladimir Kozlov wrote: If you are asking about HeapByteBufferTest.java test put it into: hotspot/test/compiler/intrinsics/unsafe/ Vladimir The existing tests for buffers are in jdk/test/java/nio/Buffer and would nice to have all the tests for this API together if possible. So I guess the issue here is that it's one test serving two purposes, it feel that there should be a new test that exercises Unsafe directly to put into the hotspot/test/compiler tree. -Alan
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 19/03/2015 16:53, Vladimir Kozlov wrote: New code did not change API of existing X-Buffer methods. It changed only how they are implementation. And already existing nio/Buffer tests should test these methods already. Okay, although I've made a note to check the test coverage to see that the existing tests cover all cases. I have no objection to this going into the compiler tree of course, just wondering whether the existing buffer tests are enough. -Alan
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
If you are asking about HeapByteBufferTest.java test put it into: hotspot/test/compiler/intrinsics/unsafe/ Vladimir On 3/19/15 2:03 AM, Andrew Haley wrote: People, please. I have been working on this for weeks. All I am waiting on now is a decision about where the test case should go. Can I have a decision so we can get this in? Thanks, Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 03/19/2015 04:33 PM, Alan Bateman wrote: On 19/03/2015 15:59, Vladimir Kozlov wrote: If you are asking about HeapByteBufferTest.java test put it into: hotspot/test/compiler/intrinsics/unsafe/ Vladimir The existing tests for buffers are in jdk/test/java/nio/Buffer and would nice to have all the tests for this API together if possible. So I guess the issue here is that it's one test serving two purposes, it feel that there should be a new test that exercises Unsafe directly to put into the hotspot/test/compiler tree. Within reason I'll do whatever you want, but I'm as sure as I can be that this test gives the new part of Unsafe a thorough workout. We need HeapByteBufferTest.java because of the substantial changes to that class. I don't know what a test of the unsafe aligned methods would do that is not done by HeapByteBufferTest. Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 03/19/2015 04:43 PM, Andrew Haley wrote: On 03/19/2015 04:33 PM, Alan Bateman wrote: On 19/03/2015 15:59, Vladimir Kozlov wrote: If you are asking about HeapByteBufferTest.java test put it into: hotspot/test/compiler/intrinsics/unsafe/ Vladimir The existing tests for buffers are in jdk/test/java/nio/Buffer and would nice to have all the tests for this API together if possible. So I guess the issue here is that it's one test serving two purposes, it feel that there should be a new test that exercises Unsafe directly to put into the hotspot/test/compiler tree. Within reason I'll do whatever you want, but I'm as sure as I can be that this test gives the new part of Unsafe a thorough workout. We need HeapByteBufferTest.java because of the substantial changes to that class. I don't know what a test of the unsafe aligned methods unaligned would do that is not done by HeapByteBufferTest. Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 16/03/15 19:16, John Rose wrote: On Mar 16, 2015, at 2:29 AM, Andrew Haley a...@redhat.com wrote: Since the option provides control for product behavior, without an explicit opt-in, it should either be a product flag or a diagnostic flag. I suggest keeping the more direct name (Use* not Disable*) and making it a diagnostic flag. OK. Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On Mar 16, 2015, at 6:19 PM, Andrew Haley a...@redhat.com wrote: On 03/16/2015 10:03 AM, Paul Sandoz wrote: I am running this patch though our JPRT test system right now. The test looks good, two comments on it: - IMO it does not need the internal PRNG FastRandom, SplittableRandom can be used instead. - I suspect that test needs to be located in the hotspot test area, which likely means it gets run on more exotic platforms. Perhaps someone with more knowledge of the test configuration and infrastructure can comment? I have a patch with all of the changes that people have asked for except this last one. I don't know who to ask. Including Stefan, who may know more. Paul.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 03/17/2015 10:38 AM, Paul Sandoz wrote: On Mar 16, 2015, at 6:19 PM, Andrew Haley a...@redhat.com wrote: On 03/16/2015 10:03 AM, Paul Sandoz wrote: I am running this patch though our JPRT test system right now. The test looks good, two comments on it: - IMO it does not need the internal PRNG FastRandom, SplittableRandom can be used instead. - I suspect that test needs to be located in the hotspot test area, which likely means it gets run on more exotic platforms. Perhaps someone with more knowledge of the test configuration and infrastructure can comment? I have a patch with all of the changes that people have asked for except this last one. I don't know who to ask. Including Stefan, who may know more. Righto. I will post a new webrev once we decide. Thanks, Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 17/03/2015 5:16 AM, John Rose wrote: On Mar 16, 2015, at 2:29 AM, Andrew Haley a...@redhat.com wrote: On 16/03/15 00:40, David Holmes wrote: Experimental options are supposed to be opt-in only via UnlockExperimentalVMOptions. You presently have the experimental UseUnalignedAccesses being turned on unconditionally on those architectures that support it. Well, it works. I guess this could be changed to be a product option, but it's only really needed for testing. And aren't product options supposed to be properly documented for all users? In other words: please don't say don't do that. Please tell me what I should do instead. All suggestions are welcome, really because I'm rather stuck. David has a point about experimental options; they have an opt-in sense to them. Actually John it has been pointed out to me that my interpretation of experimental options is not correct. It seems we already have a number of experimental options that are on by default and the experimental aspect is the turning of them off. Since the option provides control for product behavior, without an explicit opt-in, it should either be a product flag or a diagnostic flag. I suggest keeping the more direct name (Use* not Disable*) and making it a diagnostic flag. As a diagnostic flag it does not require a CCC request, since it is not for JVM customers to use. Its purpose is testing and field diagnosis by implementation engineers. Similar flags are ScavengeRootsInCode, LogEventsBufferEntries, and ParGCCardsPerStrideChunk. I think diagnostic works better here too. Thanks, David Typical uses: On platforms which support unaligned accesses, do differential performance testing to verify that the switch is correctly set for the platform. On platforms which do not, if hardware or JIT upgrades supply a faster unaligned access, do differential regression testing with the new feature in play. — John
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 03/16/2015 01:25 PM, Andrew Haley wrote: On 03/16/2015 11:36 AM, David Holmes wrote: On 16/03/2015 7:29 PM, Andrew Haley wrote: On 16/03/15 00:40, David Holmes wrote: Experimental options are supposed to be opt-in only via UnlockExperimentalVMOptions. You presently have the experimental UseUnalignedAccesses being turned on unconditionally on those architectures that support it. Well, it works. I guess this could be changed to be a product option, but it's only really needed for testing. And aren't product options supposed to be properly documented for all users? In other words: please don't say don't do that. Please tell me what I should do instead. All suggestions are welcome, really because I'm rather stuck. Either it should be an experimental option, in which case the logic that sets the flag on should check that UnlockExperimentalVMOptions is also on (else do nothing). But UnlockExperimentalVMOptions is handled by the option processing already. (Though this would still be a unusual use-case for an experimental option as the intent is that the user chooses to turn them on directly.) Or it should not be an experimental option. We need a flag which defaults to whatever is right for the platform (so it must be set in the back ends) but can be overridden by testing. What we have right now fits that requirement. I can see no way to do what I think you're asking for with the current option machinery. I certainly don't think this should be a product flag. Andrew. Hi Andrew, I think the confusion is caused by UseUnalignedAccesses flag (part of a public interface unless it is experimental - the command line options) that is used internally to communicate the cpu default to the implementation of Unsafe.unalignedAccess() method. What if: - you define an internal constant SUPPORTS_UNALIGNED_ACCESSES (I don't know how to do that correctly) in per-cpu vm_version_cpu.hpp files. - rename UseUnalignedAccesses to EmulateUnalignedAccesses (reverse the meaning, so that default can be false which must be for experimental options) - implement Unsafe.unalignedAccess() to return: SUPPORTS_UNALIGNED_ACCESSES !EmulateUnalignedAccesses ... Regards, Peter
Re: Build error on sparc was Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 03/16/2015 11:41 AM, David Holmes wrote: VM_BIG_ENDIAN does not exist in the sources. The only place it exists is in: ./bsd/makefiles/ppc.make:CFLAGS += -DVM_BIG_ENDIAN ./linux/makefiles/ppc.make:CFLAGS += -DVM_BIG_ENDIAN The expectation is that if VM_LITTLE_ENDIAN is not defined then you are big endian. Given the two options are mutually-exclusive a single flag, defined or not, covers it. Unless someone forgets, or there is a bug in the build files. This is not a theoretical possibility. This is a bug which we have seen in the last couple of months. Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 16/03/2015 7:29 PM, Andrew Haley wrote: On 16/03/15 00:40, David Holmes wrote: Experimental options are supposed to be opt-in only via UnlockExperimentalVMOptions. You presently have the experimental UseUnalignedAccesses being turned on unconditionally on those architectures that support it. Well, it works. I guess this could be changed to be a product option, but it's only really needed for testing. And aren't product options supposed to be properly documented for all users? In other words: please don't say don't do that. Please tell me what I should do instead. All suggestions are welcome, really because I'm rather stuck. Either it should be an experimental option, in which case the logic that sets the flag on should check that UnlockExperimentalVMOptions is also on (else do nothing). (Though this would still be a unusual use-case for an experimental option as the intent is that the user chooses to turn them on directly.) Or it should not be an experimental option. If it is a to be a product flag then it needs to go through the CCC process. David - Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 03/16/2015 11:36 AM, David Holmes wrote: On 16/03/2015 7:29 PM, Andrew Haley wrote: On 16/03/15 00:40, David Holmes wrote: Experimental options are supposed to be opt-in only via UnlockExperimentalVMOptions. You presently have the experimental UseUnalignedAccesses being turned on unconditionally on those architectures that support it. Well, it works. I guess this could be changed to be a product option, but it's only really needed for testing. And aren't product options supposed to be properly documented for all users? In other words: please don't say don't do that. Please tell me what I should do instead. All suggestions are welcome, really because I'm rather stuck. Either it should be an experimental option, in which case the logic that sets the flag on should check that UnlockExperimentalVMOptions is also on (else do nothing). But UnlockExperimentalVMOptions is handled by the option processing already. (Though this would still be a unusual use-case for an experimental option as the intent is that the user chooses to turn them on directly.) Or it should not be an experimental option. We need a flag which defaults to whatever is right for the platform (so it must be set in the back ends) but can be overridden by testing. What we have right now fits that requirement. I can see no way to do what I think you're asking for with the current option machinery. I certainly don't think this should be a product flag. Andrew.
Re: Build error on sparc was Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 03/16/2015 10:30 AM, Paul Sandoz wrote: On Mar 16, 2015, at 11:03 AM, Paul Sandoz paul.san...@oracle.com wrote: I am running this patch though our JPRT test system right now. I am getting a build error on spark: ... /hotspot/src/share/vm/prims/unsafe.cpp, line 335: Error: #error VM_LITTLE_ENDIAN or VM_BIG_ENDIAN must be defined. The assumption seems to be that if VM_LITTLE_ENDIAN is not defined then the platform is implicitly VM_BIG_ENDIAN. I think that we've exposed something that we should fix now. As John Rose put it, Culture of clean: Leave it nicer than you found it – Budget for technical debt, expect and welcome debt removal VM_LITTLE_ENDIAN or VM_BIG_ENDIAN should be defined on all platforms. Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
I am running this patch though our JPRT test system right now. The test looks good, two comments on it: - IMO it does not need the internal PRNG FastRandom, SplittableRandom can be used instead. - I suspect that test needs to be located in the hotspot test area, which likely means it gets run on more exotic platforms. Perhaps someone with more knowledge of the test configuration and infrastructure can comment? Paul. On Mar 13, 2015, at 7:47 PM, Andrew Haley a...@redhat.com wrote: I've tried to address all the points that have been made. I haven't changed the way the sub-words are read and written because the code I'm seeing is fairly nearly optimal right now and anything more complex runs the risk of tripping inlining limits, thus pessimizing performance. We need a general solution which isn't optimized for special cases but is decent on average and often very good. I think it's close to the best compromise we're going to get. I have changed the javadoc as requested and I have fixed the code in HeapByteBuffer so that it doesn't check the index twice for floating- point put() and get() operations. I have added a jtreg test for HeapByteBuffer which tests it pretty thoroughly. It also, as a consequence, tests the underlying Unsafe methods. I wasn't sure what a separate test for the Unsafe methods would achieve so I didn't write one. OK? http://cr.openjdk.java.net/~aph/unaligned.hotspot.6/ http://cr.openjdk.java.net/~aph/unaligned.jdk.6/ Andrew.
Build error on sparc was Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On Mar 16, 2015, at 11:03 AM, Paul Sandoz paul.san...@oracle.com wrote: I am running this patch though our JPRT test system right now. I am getting a build error on spark: ... /hotspot/src/share/vm/prims/unsafe.cpp, line 335: Error: #error VM_LITTLE_ENDIAN or VM_BIG_ENDIAN must be defined. The assumption seems to be that if VM_LITTLE_ENDIAN is not defined then the platform is implicitly VM_BIG_ENDIAN. Paul.
Re: Build error on sparc was Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 03/16/2015 11:54 AM, Andrew Haley wrote: On 03/16/2015 11:41 AM, David Holmes wrote: VM_BIG_ENDIAN does not exist in the sources. The only place it exists is in: ./bsd/makefiles/ppc.make:CFLAGS += -DVM_BIG_ENDIAN ./linux/makefiles/ppc.make:CFLAGS += -DVM_BIG_ENDIAN The expectation is that if VM_LITTLE_ENDIAN is not defined then you are big endian. Given the two options are mutually-exclusive a single flag, defined or not, covers it. Unless someone forgets, or there is a bug in the build files. This is not a theoretical possibility. This is a bug which we have seen in the last couple of months. I will change this because I really want this patch to go in, but under protest. Andrew.
Re: Build error on sparc was Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 16/03/2015 8:39 PM, Andrew Haley wrote: On 03/16/2015 10:30 AM, Paul Sandoz wrote: On Mar 16, 2015, at 11:03 AM, Paul Sandoz paul.san...@oracle.com wrote: I am running this patch though our JPRT test system right now. I am getting a build error on spark: ... /hotspot/src/share/vm/prims/unsafe.cpp, line 335: Error: #error VM_LITTLE_ENDIAN or VM_BIG_ENDIAN must be defined. The assumption seems to be that if VM_LITTLE_ENDIAN is not defined then the platform is implicitly VM_BIG_ENDIAN. I think that we've exposed something that we should fix now. As John Rose put it, Culture of clean: Leave it nicer than you found it – Budget for technical debt, expect and welcome debt removal VM_LITTLE_ENDIAN or VM_BIG_ENDIAN should be defined on all platforms. VM_BIG_ENDIAN does not exist in the sources. The only place it exists is in: ./bsd/makefiles/ppc.make:CFLAGS += -DVM_BIG_ENDIAN ./linux/makefiles/ppc.make:CFLAGS += -DVM_BIG_ENDIAN The expectation is that if VM_LITTLE_ENDIAN is not defined then you are big endian. Given the two options are mutually-exclusive a single flag, defined or not, covers it. David Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
Hi, On 03/16/2015 12:43 PM, Peter Levart wrote: On 03/16/2015 01:25 PM, Andrew Haley wrote: We need a flag which defaults to whatever is right for the platform (so it must be set in the back ends) but can be overridden by testing. What we have right now fits that requirement. I can see no way to do what I think you're asking for with the current option machinery. I certainly don't think this should be a product flag. I think the confusion is caused by UseUnalignedAccesses flag (part of a public interface unless it is experimental - the command line options) that is used internally to communicate the cpu default to the implementation of Unsafe.unalignedAccess() method. What if: - you define an internal constant SUPPORTS_UNALIGNED_ACCESSES (I don't know how to do that correctly) in per-cpu vm_version_cpu.hpp files. It can not be a constant: on some platforms it's something that you read from a configuration register. But I can do something like that. - rename UseUnalignedAccesses to EmulateUnalignedAccesses (reverse the meaning, so that default can be false which must be for experimental options) - implement Unsafe.unalignedAccess() to return: SUPPORTS_UNALIGNED_ACCESSES !EmulateUnalignedAccesses ... I'm not sure what EmulateUnalignedAccesses means; I guess it must mean that we don't generate code which does unaligned accesses. But is that what emulate unaligned accesses means to you? I'm going to go with DisableUnalignedAccesses, because I at least know what that means. :-) Thanks, Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 03/16/2015 10:03 AM, Paul Sandoz wrote: I am running this patch though our JPRT test system right now. The test looks good, two comments on it: - IMO it does not need the internal PRNG FastRandom, SplittableRandom can be used instead. - I suspect that test needs to be located in the hotspot test area, which likely means it gets run on more exotic platforms. Perhaps someone with more knowledge of the test configuration and infrastructure can comment? I have a patch with all of the changes that people have asked for except this last one. I don't know who to ask. Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On Mar 16, 2015, at 2:29 AM, Andrew Haley a...@redhat.com wrote: On 16/03/15 00:40, David Holmes wrote: Experimental options are supposed to be opt-in only via UnlockExperimentalVMOptions. You presently have the experimental UseUnalignedAccesses being turned on unconditionally on those architectures that support it. Well, it works. I guess this could be changed to be a product option, but it's only really needed for testing. And aren't product options supposed to be properly documented for all users? In other words: please don't say don't do that. Please tell me what I should do instead. All suggestions are welcome, really because I'm rather stuck. David has a point about experimental options; they have an opt-in sense to them. Since the option provides control for product behavior, without an explicit opt-in, it should either be a product flag or a diagnostic flag. I suggest keeping the more direct name (Use* not Disable*) and making it a diagnostic flag. As a diagnostic flag it does not require a CCC request, since it is not for JVM customers to use. Its purpose is testing and field diagnosis by implementation engineers. Similar flags are ScavengeRootsInCode, LogEventsBufferEntries, and ParGCCardsPerStrideChunk. Typical uses: On platforms which support unaligned accesses, do differential performance testing to verify that the switch is correctly set for the platform. On platforms which do not, if hardware or JIT upgrades supply a faster unaligned access, do differential regression testing with the new feature in play. — John
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 16/03/15 00:40, David Holmes wrote: Experimental options are supposed to be opt-in only via UnlockExperimentalVMOptions. You presently have the experimental UseUnalignedAccesses being turned on unconditionally on those architectures that support it. Well, it works. I guess this could be changed to be a product option, but it's only really needed for testing. And aren't product options supposed to be properly documented for all users? In other words: please don't say don't do that. Please tell me what I should do instead. All suggestions are welcome, really because I'm rather stuck. Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
On 16/03/15 10:03, Paul Sandoz wrote: I am running this patch though our JPRT test system right now. The test looks good, two comments on it: - IMO it does not need the internal PRNG FastRandom, SplittableRandom can be used instead. OK. - I suspect that test needs to be located in the hotspot test area, which likely means it gets run on more exotic platforms. Perhaps someone with more knowledge of the test configuration and infrastructure can comment? OK. I'll wait for an answer. Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
Andrew, Experimental options are supposed to be opt-in only via UnlockExperimentalVMOptions. You presently have the experimental UseUnalignedAccesses being turned on unconditionally on those architectures that support it. David On 14/03/2015 4:47 AM, Andrew Haley wrote: I've tried to address all the points that have been made. I haven't changed the way the sub-words are read and written because the code I'm seeing is fairly nearly optimal right now and anything more complex runs the risk of tripping inlining limits, thus pessimizing performance. We need a general solution which isn't optimized for special cases but is decent on average and often very good. I think it's close to the best compromise we're going to get. I have changed the javadoc as requested and I have fixed the code in HeapByteBuffer so that it doesn't check the index twice for floating- point put() and get() operations. I have added a jtreg test for HeapByteBuffer which tests it pretty thoroughly. It also, as a consequence, tests the underlying Unsafe methods. I wasn't sure what a separate test for the Unsafe methods would achieve so I didn't write one. OK? http://cr.openjdk.java.net/~aph/unaligned.hotspot.6/ http://cr.openjdk.java.net/~aph/unaligned.jdk.6/ Andrew.
Re: 8026049: (bf) Intrinsify ByteBuffer.put{Int, Double, Float, ...} methods
Hi Andrew, Looks good to me. Rémi On 03/13/2015 07:47 PM, Andrew Haley wrote: I've tried to address all the points that have been made. I haven't changed the way the sub-words are read and written because the code I'm seeing is fairly nearly optimal right now and anything more complex runs the risk of tripping inlining limits, thus pessimizing performance. We need a general solution which isn't optimized for special cases but is decent on average and often very good. I think it's close to the best compromise we're going to get. I have changed the javadoc as requested and I have fixed the code in HeapByteBuffer so that it doesn't check the index twice for floating- point put() and get() operations. I have added a jtreg test for HeapByteBuffer which tests it pretty thoroughly. It also, as a consequence, tests the underlying Unsafe methods. I wasn't sure what a separate test for the Unsafe methods would achieve so I didn't write one. OK? http://cr.openjdk.java.net/~aph/unaligned.hotspot.6/ http://cr.openjdk.java.net/~aph/unaligned.jdk.6/ Andrew.
