subject:"RFR\: 8224974\: Implement JEP 352"




On 8/7/19 4:29 PM, Andrew Dinn wrote:

On 07/08/2019 13:04, Dmitry Chuyko wrote:

On 8/7/19 2:02 PM, Dmitry Chuyko wrote:

Andrew,

New code is buildable and new MapFail test passes on different
platforms except it fails on linux i386:

Ah, that even was x86_64 (sorry, mixed up results from automated
system). I'll try to reproduce it by hand to see if there are any
additional details.

Whew! Thank goodness it is x86_64. You had me very worried when you said
this was i386 :-)

So, this is indeed a problem with the test. The internal detail message
for the IOException will vary according to the Linux kernel release. If
MAP_SYNC is not supported by the Linux kernel it will embed the message
you saw. If it is supported the message will be the one the test checks for.

I think the best thing is for the test not to bother checking for a
specific message. The important thing is that we should get either
IOException or UnsupportedOperationException. Checking for the message
is not really necessary. I will remove this check from the test.


Ok, it will reduce complexity and likely let the test pass while still 
checking the same thing.


-Dmitry



regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

The test fails on some machines but does not fail on others, all 4.x 
kernels. The possible problem may be when build host and run host are 
different machines. This seems to be related to map0() implementation in 
FileChannelImpl.c in case MAP_SYNC and MAP_SHARED_VALIDATE are not 
defined (or defined).


I also recommend to print original ioe stacktrace in the test. Adding 
such gives us useful information like this:


java.io.IOException: Invalid argument

at java.base/sun.nio.ch.FileChannelImpl.map0(Native Method)
at java.base/sun.nio.ch.FileChannelImpl.map(FileChannelImpl.java:1062)
at MapFail.main(MapFail.java:48)
at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native 
Method)
at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

at java.base/java.lang.reflect.Method.invoke(Method.java:565)
at 
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)

at java.base/java.lang.Thread.run(Thread.java:830)
java.lang.Exception: unexpected message for IOExceptionInvalid argument
at MapFail.main(MapFail.java:61)
at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native 
Method)
at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

at java.base/java.lang.reflect.Method.invoke(Method.java:565)
at 
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)

at java.base/java.lang.Thread.run(Thread.java:830)
Caused by: java.io.IOException: Invalid argument
at java.base/sun.nio.ch.FileChannelImpl.map0(Native Method)
at java.base/sun.nio.ch.FileChannelImpl.map(FileChannelImpl.java:1062)
at MapFail.main(MapFail.java:48)
... 6 more

-Dmitry

On 8/7/19 3:04 PM, Dmitry Chuyko wrote:

On 8/7/19 2:02 PM, Dmitry Chuyko wrote:

Andrew,

New code is buildable and new MapFail test passes on different 
platforms except it fails on linux i386:


Ah, that even was x86_64 (sorry, mixed up results from automated 
system). I'll try to reproduce it by hand to see if there are any 
additional details.


-Dmitry



--System.err:(12/712)--
java.lang.Exception: unexpected message for IOExceptionInvalid argument
    at MapFail.main(MapFail.java:60)
    at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native 
Method)
    at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

    at java.base/java.lang.reflect.Method.invoke(Method.java:565)
    at 
com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:246)

    at java.base/java.lang.Thread.run(Thread.java:830)

First there is a problem with the test,
and a minor test issue is it would be good to add ": " before actual 
unexpected message.


-Dmitry

On 8/7/19 12:31 PM, Dmitry Chuyko wrote:

On 8/6/19 6:58 PM, Andrew Dinn wrote:

..
No this behaviour is not currently tested. However, the only client at
present will never exercise that path so it is not critical to test it
now. I'd be happy to address testing of this behaviour as part of a
follow-up JIRA issue if you would be so good as to raise it. I say 
that
because I would very much like to get this functionality into a 
release

to simplify more extensive testing by Red Hat's middleware teams.

It sounds reasonable, I'll create a tiny RFE after you push the JEP.



New MapFail test succeeds if proper IOException or any
UnsupportedOperationException was caught but it aren't those 
situations
actually 2 different ones that require distinct checks? If you say 
that
is the situation when results depend on Linux version, it makes 
sense at

least to put a comment in the test because it's really not trivial.
The documentation of the API under test makes it clear that both 
errors
can occur and under what circumstances. However, a note in the test 
will

certainly do no harm. I will insert one before checking in the patch.


Can PmemTest check IOException with message "map with mode MAP_SYNC
unsupported" as a part of expected behavior, not just showing a test
failure?

I don't see any need for this now that MapFail has been provided. Wit
that alterative in place for checking map failures on non-DAX file
syetems PmemTest is now primarily intended to test behaviour with a 
DAX
file system which it expects to have been set up in advance as 
described

in the main comment. So, the scenario you describe is not really an
intended usage and I woudl argue that a failure is the right way to
signal that.


OK, finally got it, thank you.

-Dmitry



regards,


Andrew Dinn
---
Senior Principal Software Engineer

Re: RFR: 8224974: Implement JEP 352

2019-08-07 Thread Andrew Dinn

On 07/08/2019 13:04, Dmitry Chuyko wrote:
> On 8/7/19 2:02 PM, Dmitry Chuyko wrote:
>> Andrew,
>>
>> New code is buildable and new MapFail test passes on different
>> platforms except it fails on linux i386:
> 
> Ah, that even was x86_64 (sorry, mixed up results from automated
> system). I'll try to reproduce it by hand to see if there are any
> additional details.
Whew! Thank goodness it is x86_64. You had me very worried when you said
this was i386 :-)

So, this is indeed a problem with the test. The internal detail message
for the IOException will vary according to the Linux kernel release. If
MAP_SYNC is not supported by the Linux kernel it will embed the message
you saw. If it is supported the message will be the one the test checks for.

I think the best thing is for the test not to bother checking for a
specific message. The important thing is that we should get either
IOException or UnsupportedOperationException. Checking for the message
is not really necessary. I will remove this check from the test.

regards,

Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

On 8/7/19 2:02 PM, Dmitry Chuyko wrote:

Andrew,

New code is buildable and new MapFail test passes on different
platforms except it fails on linux i386:

Ah, that even was x86_64 (sorry, mixed up results from automated
system). I'll try to reproduce it by hand to see if there are any
additional details.

-Dmitry

--System.err:(12/712)--
java.lang.Exception: unexpected message for IOExceptionInvalid argument
at MapFail.main(MapFail.java:60)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
Method)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

at java.base/java.lang.reflect.Method.invoke(Method.java:565)
at
com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:246)

at java.base/java.lang.Thread.run(Thread.java:830)

First there is a problem with the test,
and a minor test issue is it would be good to add ": " before actual
unexpected message.

-Dmitry

On 8/7/19 12:31 PM, Dmitry Chuyko wrote:

On 8/6/19 6:58 PM, Andrew Dinn wrote:

..
No this behaviour is not currently tested. However, the only client at
present will never exercise that path so it is not critical to test it
now. I'd be happy to address testing of this behaviour as part of a
follow-up JIRA issue if you would be so good as to raise it. I say that
because I would very much like to get this functionality into a release
to simplify more extensive testing by Red Hat's middleware teams.

It sounds reasonable, I'll create a tiny RFE after you push the JEP.

New MapFail test succeeds if proper IOException or any
UnsupportedOperationException was caught but it aren't those
situations
actually 2 different ones that require distinct checks? If you say
that
is the situation when results depend on Linux version, it makes
sense at

least to put a comment in the test because it's really not trivial.

The documentation of the API under test makes it clear that both errors
can occur and under what circumstances. However, a note in the test
will

certainly do no harm. I will insert one before checking in the patch.

Can PmemTest check IOException with message "map with mode MAP_SYNC
unsupported" as a part of expected behavior, not just showing a test
failure?

in the main comment. So, the scenario you describe is not really an
intended usage and I woudl argue that a failure is the right way to
signal that.

OK, finally got it, thank you.

-Dmitry

regards,

Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

Andrew,

New code is buildable and new MapFail test passes on different platforms
except it fails on linux i386:

at java.base/java.lang.reflect.Method.invoke(Method.java:565)
at
com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:246)

at java.base/java.lang.Thread.run(Thread.java:830)

First there is a problem with the test,
and a minor test issue is it would be good to add ": " before actual
unexpected message.

-Dmitry

On 8/7/19 12:31 PM, Dmitry Chuyko wrote:

On 8/6/19 6:58 PM, Andrew Dinn wrote:

It sounds reasonable, I'll create a tiny RFE after you push the JEP.

New MapFail test succeeds if proper IOException or any
UnsupportedOperationException was caught but it aren't those situations
actually 2 different ones that require distinct checks? If you say that
is the situation when results depend on Linux version, it makes
sense at

least to put a comment in the test because it's really not trivial.

The documentation of the API under test makes it clear that both errors
can occur and under what circumstances. However, a note in the test will
certainly do no harm. I will insert one before checking in the patch.

Can PmemTest check IOException with message "map with mode MAP_SYNC
unsupported" as a part of expected behavior, not just showing a test
failure?

I don't see any need for this now that MapFail has been provided. Wit
that alterative in place for checking map failures on non-DAX file
syetems PmemTest is now primarily intended to test behaviour with a DAX
file system which it expects to have been set up in advance as described
in the main comment. So, the scenario you describe is not really an
intended usage and I woudl argue that a failure is the right way to
signal that.

OK, finally got it, thank you.

-Dmitry

regards,

Re: RFR: 8224974: Implement JEP 352

2019-08-07 Thread Alan Bateman


On 06/08/2019 09:09, Andrew Dinn wrote:

:
The unmapper code is not strictly 'new' as regards its reliance on
synchronization. It merely follows and repeats the pattern employed in
the prior code that it has generalized (by splitting the original
Unmapper into two distinct flavours of subclass).

If this poses a problem for Loom then it is a separate issue form the
one this JEP addresses. I think you should raise a new issue for that
change (just as you would have had to do before this change). I am sure
Alan Bateman will be happy to consider your proposal. Indeed, I would be
happy to implement it given his approval -- or leave it to you to do so
if you prefer.

I don't think we need to be concerned with any of this at this time. The 
unmapper is run by the reference handler thread. Also the 
synchronization here is for the counters so not the same thing as doing 
a blocking I/O operation while holding a monitor. At some point we'll 
examine all the file I/O operations as some of these are candidates for 
managed blockers, others are candidates for alternative implementations 
- there are bigger issues to resolve first and we've been trying to 
avoid carrying too many changes due to the complexity and effort needed 
to keep them in sync with the main line.


-Alan

Re: RFR: 8224974: Implement JEP 352


On 8/6/19 7:09 PM, Andrew Dinn wrote:

Hello Dmitry,

On 06/08/2019 15:25, Dmitry Chuyko wrote:

One quick question about synchronization in unmappers. One of
preliminary steps for Loom was to replace monitor usage by j.u.c locks
for I/O to let fibers release carrier threads. For instance see
JDK-8222774. Does it make sense to do the same in your new unmappers code?
. . .
[1] https://bugs.openjdk.java.net/browse/JDK-8222774

The unmapper code is not strictly 'new' as regards its reliance on
synchronization. It merely follows and repeats the pattern employed in
the prior code that it has generalized (by splitting the original
Unmapper into two distinct flavours of subclass).

If this poses a problem for Loom then it is a separate issue form the
one this JEP addresses. I think you should raise a new issue for that
change (just as you would have had to do before this change). I am sure
Alan Bateman will be happy to consider your proposal. Indeed, I would be
happy to implement it given his approval -- or leave it to you to do so
if you prefer.


Agree, Loom has a long road to go. So I suppose such a change will be a 
part of larger work in sun.nio, and I or one of my colleagues will be 
happy to participate. Changes will probably be straightforward (e.g. 
JDK-8222882) but this synchronization is not covered by regression tests 
so I believe in this case you'll help to retry some of your ad-hoc 
testing or maybe some application tests you know about.


-Dmitry



regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352


On 8/6/19 6:58 PM, Andrew Dinn wrote:

..
No this behaviour is not currently tested. However, the only client at
present will never exercise that path so it is not critical to test it
now. I'd be happy to address testing of this behaviour as part of a
follow-up JIRA issue if you would be so good as to raise it. I say that
because I would very much like to get this functionality into a release
to simplify more extensive testing by Red Hat's middleware teams.

It sounds reasonable, I'll create a tiny RFE after you push the JEP.



New MapFail test succeeds if proper IOException or any
UnsupportedOperationException was caught but it aren't those situations
actually 2 different ones that require distinct checks? If you say that
is the situation when results depend on Linux version, it makes sense at
least to put a comment in the test because it's really not trivial.

The documentation of the API under test makes it clear that both errors
can occur and under what circumstances. However, a note in the test will
certainly do no harm. I will insert one before checking in the patch.


Can PmemTest check IOException with message "map with mode MAP_SYNC
unsupported" as a part of expected behavior, not just showing a test
failure?

I don't see any need for this now that MapFail has been provided. Wit
that alterative in place for checking map failures on non-DAX file
syetems PmemTest is now primarily intended to test behaviour with a DAX
file system which it expects to have been set up in advance as described
in the main comment. So, the scenario you describe is not really an
intended usage and I woudl argue that a failure is the right way to
signal that.


OK, finally got it, thank you.

-Dmitry



regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

Hello Dmitry,

On 06/08/2019 15:25, Dmitry Chuyko wrote:
> One quick question about synchronization in unmappers. One of
> preliminary steps for Loom was to replace monitor usage by j.u.c locks
> for I/O to let fibers release carrier threads. For instance see
> JDK-8222774. Does it make sense to do the same in your new unmappers code?
> . . .
> [1] https://bugs.openjdk.java.net/browse/JDK-8222774
The unmapper code is not strictly 'new' as regards its reliance on
synchronization. It merely follows and repeats the pattern employed in
the prior code that it has generalized (by splitting the original
Unmapper into two distinct flavours of subclass).

If this poses a problem for Loom then it is a separate issue form the
one this JEP addresses. I think you should raise a new issue for that
change (just as you would have had to do before this change). I am sure
Alan Bateman will be happy to consider your proposal. Indeed, I would be
happy to implement it given his approval -- or leave it to you to do so
if you prefer.

regards,

Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

Hello Dmitry,

On 06/08/2019 15:50, Dmitry Chuyko wrote:
> Thanks for previous detailed explanations, and great, there are now
> checks for exceptions in webrev.11. Let me add just a few comments about
> that.

Sure, no problem. Thank you for looking at the patch.

> New
> Unsafe.writebackMemory()/checkWritebackMemory()/checkWritebackEnabled()
> @throws RuntimeException "if memory writeback is not supported...".
> There is no test that check this behavior of Unsafe class while it seems
> to be relatively simple as inner check actually operates with a constant
> value.

No this behaviour is not currently tested. However, the only client at
present will never exercise that path so it is not critical to test it
now. I'd be happy to address testing of this behaviour as part of a
follow-up JIRA issue if you would be so good as to raise it. I say that
because I would very much like to get this functionality into a release
to simplify more extensive testing by Red Hat's middleware teams.

> New MapFail test succeeds if proper IOException or any
> UnsupportedOperationException was caught but it aren't those situations
> actually 2 different ones that require distinct checks? If you say that
> is the situation when results depend on Linux version, it makes sense at
> least to put a comment in the test because it's really not trivial.

The documentation of the API under test makes it clear that both errors
can occur and under what circumstances. However, a note in the test will
certainly do no harm. I will insert one before checking in the patch.

> Can PmemTest check IOException with message "map with mode MAP_SYNC
> unsupported" as a part of expected behavior, not just showing a test
> failure?

I don't see any need for this now that MapFail has been provided. Wit
that alterative in place for checking map failures on non-DAX file
syetems PmemTest is now primarily intended to test behaviour with a DAX
file system which it expects to have been set up in advance as described
in the main comment. So, the scenario you describe is not really an
intended usage and I woudl argue that a failure is the right way to
signal that.

regards,

Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-08-06 Thread Dmitry Chuyko


Hi Andrew,

Thanks for previous detailed explanations, and great, there are now 
checks for exceptions in webrev.11. Let me add just a few comments about 
that.


On 8/1/19 12:49 PM, Andrew Dinn wrote:

Hello Dmitry,

On 01/08/2019 05:09, Dmitry Chuyko wrote:

Just a small comment about the tests. As you can see, some of tests in
jdk/java/nio/channels/FileChannel check various modes, arguments, and
expected exceptions. E.g. see MapTest, Mode and Args.

I noticed that in your changes for the new map mode there are new
exceptions thrown in different situations. For example, when the
required mode is not supported, or it does not fit. In particular, this
should happen correctly on systems that do not support nvram. Have you
considered the possibility of extending or adding tests for this behavior?

I agree that these failure cases need better test coverage and
automation. However, that is not to say they have not been tested. All
current success and failure cases can be exercised by the current mmap
test (PmemTest) if run in the correct circumstance. Unfortunately,
automatic testing is not straightforward.


New Unsafe.writebackMemory()/checkWritebackMemory()/checkWritebackEnabled() @throws 
RuntimeException "if memory writeback is not supported...". There is no test 
that check this behavior of Unsafe class while it seems to be relatively simple as inner 
check actually operates with a constant value.

New MapFail test succeeds if proper IOException or any 
UnsupportedOperationException was caught but it aren't those situations 
actually 2 different ones that require distinct checks? If you say that is the 
situation when results depend on Linux version, it makes sense at least to put 
a comment in the test because it's really not trivial.

Can PmemTest check IOException with message "map with mode MAP_SYNC 
unsupported" as a part of expected behavior, not just showing a test failure?

-Dmitry



1) On x86_64 where MAP_SYNC is supported test PMemTest includes
instructions to exercise a successful map from a real or emulated DAX
file system. It can also be used (and has been used) to exercise the
IOException failure path in the case where the file it tries to open
belongs to a non-DAX file system (see line 99).

Note that testing for success or failure cannot be done automatically
using the test as it currently stands. Testing for a successful map
requires mounting a real or emulated DAX file system at a known mount
point and creating a writable dir to hold the mapped file. Testing for
the IOException failure requires either setting up an equivalent non-DAX
file system mount or editing the test to open the file in a non-DAX file
system dir like, say, /tmp.

A new, separate test for the IOException failure could be automated on
x86_64 by trying to map a file located in /tmp (on the assumption that
/tmp is not mounted as a DAX file system). Of course, at present this
will only give the IOException result when MAP_SYNC is supported. Given
a suitably old Linux/x86_64, UnsupportedOperationException could also be
thrown.

2) On AArch64 where MAP_SYNC is not currently supported PmemTest clearly
cannot be used to exercise the success path. However, it can be used
(and has been used) to exercise the UnsupportedOperationException
failure path.

A check for the UnsupportedOperationException failure could be automated
on AArch64 by a new test as described above. Of course, once MAP_SYNC
support arrives in a new Linux/AArch64 it would also become for
IOException to be thrown.

So, to sum up, it would be possible to add a new, automatic test that
checks for one or other failure occurring. I am happy to add such a test
to the next webrev.

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-08-06 Thread Dmitry Chuyko


Hi Andrew,

One quick question about synchronization in unmappers. One of 
preliminary steps for Loom was to replace monitor usage by j.u.c locks 
for I/O to let fibers release carrier threads. For instance see 
JDK-8222774. Does it make sense to do the same in your new unmappers code?


-Dmitry

[1] https://bugs.openjdk.java.net/browse/JDK-8222774

On 8/6/19 4:57 PM, Andrew Dinn wrote:

On 06/08/2019 13:44, Aleksey Shipilev wrote:

Ah, that is exactly what I wanted. Good then, scratch the rest of my comments.
. . .
I thought that translating two separate (and statically bound) Unsafe calls, 
hooking them up to
separate Unsafe leaf entries, and then suddenly going into a single StubRoutine 
call with dynamic
argument that dispatches at runtime is a bit awkward. I would have expected it 
to end up with two
separate StubRoutines as well. Again, I have no strong opinion about this.

Ok, thanks for clarifying. Inertia dictates I leave the stubs as is :-)


Yes, this looks cleaner. The declarations can be a bit less crowded:

   static address data_cache_writeback()  { return _data_cache_writeback; }
   static address data_cache_writeback_sync() { return 
_data_cache_writeback_sync; }

   typedef void (*DataCacheWritebackStub)(void *);
   static DataCacheWritebackStub DataCacheWriteback_stub() { return ...

   typedef void (*DataCacheWritebackSyncStub)(bool);
   static DataCacheWritebackSyncStub DataCacheWritebackSync_stub() { return ...
   . . .

   http://cr.openjdk.java.net/~adinn/8224974/webrev.11

Looks good.

Ok, I'll fold this and the other format errors you identified into the
next patch.


If I could please get a nod from Alan Bateman (and assuming I don't
receive further comments from other reviewers) I'll push that next patch.

Any more for any more ... ?

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

On 06/08/2019 13:44, Aleksey Shipilev wrote:
> Ah, that is exactly what I wanted. Good then, scratch the rest of my comments.
> . . .
> I thought that translating two separate (and statically bound) Unsafe calls, 
> hooking them up to
> separate Unsafe leaf entries, and then suddenly going into a single 
> StubRoutine call with dynamic
> argument that dispatches at runtime is a bit awkward. I would have expected 
> it to end up with two
> separate StubRoutines as well. Again, I have no strong opinion about this.

Ok, thanks for clarifying. Inertia dictates I leave the stubs as is :-)

> Yes, this looks cleaner. The declarations can be a bit less crowded:
> 
>   static address data_cache_writeback()  { return _data_cache_writeback; }
>   static address data_cache_writeback_sync() { return 
> _data_cache_writeback_sync; }
> 
>   typedef void (*DataCacheWritebackStub)(void *);
>   static DataCacheWritebackStub DataCacheWriteback_stub() { return ...
> 
>   typedef void (*DataCacheWritebackSyncStub)(bool);
>   static DataCacheWritebackSyncStub DataCacheWritebackSync_stub() { return ...
>   . . .
>>   http://cr.openjdk.java.net/~adinn/8224974/webrev.11
> 
> Looks good.

Ok, I'll fold this and the other format errors you identified into the
next patch.


If I could please get a nod from Alan Bateman (and assuming I don't
receive further comments from other reviewers) I'll push that next patch.

Any more for any more ... ?

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-08-06 Thread Aleksey Shipilev

On 8/6/19 2:12 PM, Andrew Dinn wrote:
> On 01/08/2019 12:16, Aleksey Shipilev wrote:
>> On 7/31/19 12:55 PM, Andrew Dinn wrote:
>> I am more concerned that the writeback call enters the pre sync stub 
>> unnecessarily.
> 
> The stub? I hope you mean when executing the native call as opposed to
> the JITted intrinsic? The stub is only called on a cold path when a
> native call proper happens. By contrast, the intrinsic translation for
> CacheWBPreSync on AArch64 and x86_64 does not insert any instructions
> into the generated code (and most especially not a call to the stub).

Ah, that is exactly what I wanted. Good then, scratch the rest of my comments.

>> This is not a strong requirement from my side. I do believe it would make 
>> code a bit more
>> straight-forward.
> 
> Am I missing something here? Or did you simply miss that the intrinsic
> translation inserts no code for the presync?

I thought that translating two separate (and statically bound) Unsafe calls, 
hooking them up to
separate Unsafe leaf entries, and then suddenly going into a single StubRoutine 
call with dynamic
argument that dispatches at runtime is a bit awkward. I would have expected it 
to end up with two
separate StubRoutines as well. Again, I have no strong opinion about this.

 === src/hotspot/share/prims/unsafe.cpp

 Do we really need this function pointer mess?

  457   void (*wb)(void *);
  458   void *a = addr_from_java(line);
  459   wb = (void (*)(void *)) StubRoutines::data_cache_writeback();
  460   assert(wb != NULL, "generate writeback stub!");
  461   (*wb)(a);

 Seems easier to:

...

>   static address unsafe_arraycopy() { return _unsafe_arraycopy; }
>   typedef void (*UnsafeArrayCopyStub)(const void* src, void* dst, size_t
> count);
>   static UnsafeArrayCopyStub UnsafeArrayCopy_stub() { return
> CAST_TO_FN_PTR(UnsafeArrayCopyStub,  _unsafe_arraycopy); }
> 
> I have provided similar magic to hide the function pointer details for
> the writeback and writeback_sync stubs.

Yes, this looks cleaner. The declarations can be a bit less crowded:

  static address data_cache_writeback()  { return _data_cache_writeback; }
  static address data_cache_writeback_sync() { return 
_data_cache_writeback_sync; }

  typedef void (*DataCacheWritebackStub)(void *);
  static DataCacheWritebackStub DataCacheWriteback_stub() { return ...

  typedef void (*DataCacheWritebackSyncStub)(bool);
  static DataCacheWritebackSyncStub DataCacheWritebackSync_stub() { return ...

>   http://cr.openjdk.java.net/~adinn/8224974/webrev.11

Looks good.

Minor nits (no need for another webrev):

*) Not sure if the only copyright line change is needed in 
src/hotspot/cpu/aarch64/globals_aarch64.hpp.

*) Indenting is a bit off at L109 in 
src/hotspot/cpu/aarch64/vm_version_aarch64.hpp:

 108   static int cpu_revision()   { return _revision; }
 109   static bool supports_dcpop()  { return _dcpop; }

*) Excess new line added at the end of src/hotspot/os/bsd/os_bsd.cpp?

*) Indenting is off in backslashes in src/hotspot/share/runtime/globals.hpp:

2444
 \
2445   develop(bool, TraceMemoryWriteback, false, \
2446   "Trace memory writeback operations") 
 \
2447
 \

*) Unnecessary newline at L827 in src/hotspot/share/runtime/os.hpp?

 826   // support for mapping non-volatile memory using MAP_SYNC
 827
 828   static bool supports_map_sync();

*) These declarations are too dense in 
src/java.base/share/classes/jdk/internal/misc/Unsafe.java:

 998 /**
 999  * primitive operation forcing writeback of a single cache line.
1000  *
1001  * @param address
1002  *the start address of the cache line to be written back
1003  */
1004 // native used to write back an individual cache line starting at
1005 // the supplied address
1006 @HotSpotIntrinsicCandidate
1007 private native void writeback0(long address);
1008 // native used to serialise writeback operations relative to
1009 // preceding memory writes
1010 @HotSpotIntrinsicCandidate
1011 private native void writebackPreSync0();
1012 // native used to serialise writeback operations relative to
1013 // following memory writes
1014 @HotSpotIntrinsicCandidate
1015 private native void writebackPostSync0();

Suggestion:

 /**
  * Force write back an individual cache line.
  *
  * @param address
  *the start address of the cache line to be written back
  */
 @HotSpotIntrinsicCandidate
 private native void writeback0(long address);

 /**
  * Serialize writeback operations relative to preceding memory writes.
  */
 @HotSpotIntrinsicCandidate
 private native void

Re: RFR: 8224974: Implement JEP 352

Hi Aleksey/Boris,

This is a response to both your last review posts. New webrev link is at
the end.

On 01/08/2019 12:16, Aleksey Shipilev wrote:
> On 7/31/19 12:55 PM, Andrew Dinn wrote:
  . . .
> I am more concerned that the writeback call enters the pre sync stub 
> unnecessarily.

The stub? I hope you mean when executing the native call as opposed to
the JITted intrinsic? The stub is only called on a cold path when a
native call proper happens. By contrast, the intrinsic translation for
CacheWBPreSync on AArch64 and x86_64 does not insert any instructions
into the generated code (and most especially not a call to the stub).
Here it is:

  instruct cacheWBPreSync()
  %{
predicate(VM_Version::supports_data_cache_line_flush());
match(CacheWBPreSync);

ins_cost(100);
format %{"cache wb presync" %}
ins_encode %{
  __ cache_wbsync(true);
%}
ins_pipe(pipe_slow); // XXX
  %}

  void MacroAssembler::cache_wbsync(bool is_pre)
  {
assert(VM_Version::supports_clflush(), "clflush should be available");
bool optimized = VM_Version::supports_clflushopt();
bool no_evict = VM_Version::supports_clwb();

// pick the correct implementation

if (!is_pre && (optimized || no_evict)) {
  // need an sfence for post flush when using clflushopt or clwb
  // otherwise no no need for any synchroniaztion

  sfence();
}
  }

> I had the idea to do this more efficiently, and simplify code at the same 
> time: how about emitting
> CacheWBPreSync nodes, but emitting nothing for them in .ad matchers? That 
> would leave generic code
> generic, and architectures would then be able to avoid the stub altogether 
> for pre sync code. This
> would simplify current stub generators too, I think: you don't need to pass 
> arguments to them.

I believe the intrinsic behaviour you are asking for is effectively what
is implemented (as shown above). The .ad match rules for the PreSync and
PostSync nodes both call MacroAssembler::cache_wbsync. For pre-sync it
emits nothing. For post-sync it emits sfence when the writeback is
implemented using clwb or clflushopt and nothing if writeback relies on
clflush.

> This leaves calling via Unsafe. I believe pulling up the isPre choice to the 
> stub generation time
> would be beneficial. That is, generate *two* stubs: 
> StubRoutines::data_cache_writeback_pre_sync()
> and StubRoutines::data_cache_writeback_post_sync(). If arch does not need the 
> pre_sync, generate nop
> version of pre_sync().

I don't really see any point to doing this. We can generate two stubs,
one executing a nop and one executing a nop/sfence according to need. Or
we can have one stub with a branch on the sync type and branch targets
that execute either a nop or a nop/sfence as needed. The difference in
performance of the stub is minor and irrelevant. The difference in
generation time and memory use is minor and irrelevant. What are you
trying to gain here?

> This is not a strong requirement from my side. I do believe it would make 
> code a bit more
> straight-forward.

Am I missing something here? Or did you simply miss that the intrinsic
translation inserts no code for the presync?

>>> === src/hotspot/cpu/x86/assembler_x86.cpp
>>>
>>> It feels like these comments are redundant, especially L8630 and L8646 
>>> which mention magic values
>>> "6" and "7", not present in the code:
> 
> ...
> 
>> 8624   // 0x66 is instruction prefix
>>
>> 8627   // 0x0f 0xAE is opcode family
>>
>> 8630   // rdi == 7 is extended opcode byte
>> . . .
>>
>> Given that the code is simply stuffing numbers (whether supplied as
>> literals or as symbolic constants) into a byte stream I think these
>> comments are a help when it comes to cross-checking each specific
>> assembly against the corresponding numbers declared in the Intel
>> manuals. So, I don't really want to remove them. Would you prefer me to
>> reverse the wording as above?
> 
> I was merely commenting on the style: the rest of the file does not have 
> comments like that. The
> positions of prefixes, opcode families, etc is kinda implied by the code 
> shape.

Yes, I too noticed that the rest of the file does not have any such
comments :-]

Given the highly variable shape of x86 machine code, I don't see any
reason not to start remedying that omission, even if the remedy is only
piecemeal. Commenting may not be a great help to maintainers who know
the code and ISA really well but they are not the only audience. Even in
that specific case the comments provide a sanity check.

>>> === src/hotspot/cpu/x86/macroAssembler_x86.cpp
>>   // prefer clwb (potentially parallel writeback without evict)
>>   // otherwise prefer clflushopt (potentially parallel writeback
>>   // with evict)
>>   // otherwise fallback on clflush (serial writeback with evict)
>>
>> In the second case the comment is redundant because the need for an
>> sfence is covered by the existing comment inside the if:
>>
>> // need an sfence for post flush when

Re: RFR: 8224974: Implement JEP 352

2019-08-01 Thread Andrew Dinn

Hi Boris,

On 31/07/2019 13:01, Boris Ulasevich wrote:
> I did a quick check of the change across our platforms. Arm32 and x86_64
> built successfully. But I see it fails due to minor issues on aarch64
> and x86_32 with webrev.09.
> Can you please have a look at this?
> 
>> src/hotspot/cpu/aarch64/aarch64.ad:2202:1: error: expected ‘;’ before
> ‘}’ token
>> src/hotspot/cpu/x86/macroAssembler_x86.cpp:9925: undefined reference
> to `Assembler::clflush(Address)'
The AArch64 error was simply a missing semi-colon. With that corrected
AArch64 now builds and runs as expected (i.e. it fails the PMem support
test with an UnsupportedOperationException).

The second error is happening because the calling method
MacroAssembler::cache_wb has not been guarded with #ifdef _LP64 (the
same applies for MacroAssembler::cache_wbsync). Note that cache line
writeback via Unsafe.writeBackMemory is only expected to work on Intel
x86_64 so these two methods only get called from x86_64-specific code
(x86_64.ad and stuGenerator_x86_64.cpp).

So, the solution to this specific problem is to add #ifdef _LP64 around
the declaration and implementation of these two methods. At the same
time it would be helpful to remove the redundant #ifdef _LP64/#endif
that I for some strange reason inserted around the definitions, but not
the declarations, of clflushopt and clwb (that didn't help when I was
trying to work out what was going wrong).

However, a related problem also needs fixing. The Java code for method
Unsafe.writebackMemory only proceeds when the data cache line writeback
unit size (value of field UnsafeConstants.DATA_CACHE_LINE_FLUSH_SIZE) is
non-zero. Otherwise it throws an exception. On x86_32 that field /must/
be zero. The native methods which Unsafe calls out to and the intrinsics
which replace the native calls are not implemented on x86_32.

The field from which the value of the Java constant is derived is
currently initialised using CPU-specific information in
vm_version_x86.cpp as follows

   if (os::supports_map_sync()) {
 // publish data cache line flush size to generic field, otherwise
 // let if default to zero thereby disabling writeback
 _data_cache_line_flush_size =
_cpuid_info.std_cpuid1_ebx.bits.clflush_size * 8;
   }

i.e. writeback is enabled on x86 when the operating is known to be
capable of supporting MAP_SYNC. os_linux.cpp returns true for that call,
irrespective of whether this is 32 or 64 bit linux. The rationale is
that any Linux is capable of supporting map_sync (by contrast Windows,
Solaris, AIX etc currently return false). So, the above assignment also
needs to be guarded by #ifdef _LP64 in order to ensure that writeback is
never attempted on x86_32.

Thank you for spotting these errors. I will add the relevant fixes to
the next patch and add you as a reviewer.

regards,

Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-08-01 Thread Aleksey Shipilev

On 7/31/19 12:55 PM, Andrew Dinn wrote:
>> So if pre wbsync is no-op, why do we need to handle it everywhere? We seem 
>> to be falling through all
>> the way to the stub to do nothing there, maybe we should instead cut much 
>> earlier, e.g. when
>> inlining Unsafe.writeBackPresync0? Would it be better to not emit 
>> CacheWBPreSync at all?
> 
> The pre sync is definitely not needed at present. However, I put it
> there because I didn't know for sure if some future port of this
> capability (e.g. to ppc) might need to sync prior writes before writing
> back cache lines. [Indeed, the old Intel documentation stated that
> pre-sync was needed on x86 for clflush to be safe but it is definitely
> not needed.]

I am more concerned that the writeback call enters the pre sync stub 
unnecessarily.

I had the idea to do this more efficiently, and simplify code at the same time: 
how about emitting
CacheWBPreSync nodes, but emitting nothing for them in .ad matchers? That would 
leave generic code
generic, and architectures would then be able to avoid the stub altogether for 
pre sync code. This
would simplify current stub generators too, I think: you don't need to pass 
arguments to them.

This leaves calling via Unsafe. I believe pulling up the isPre choice to the 
stub generation time
would be beneficial. That is, generate *two* stubs: 
StubRoutines::data_cache_writeback_pre_sync()
and StubRoutines::data_cache_writeback_post_sync(). If arch does not need the 
pre_sync, generate nop
version of pre_sync().

This is not a strong requirement from my side. I do believe it would make code 
a bit more
straight-forward.

>> === src/hotspot/cpu/x86/assembler_x86.cpp
>>
>> It feels like these comments are redundant, especially L8630 and L8646 which 
>> mention magic values
>> "6" and "7", not present in the code:

...

> 8624   // 0x66 is instruction prefix
> 
> 8627   // 0x0f 0xAE is opcode family
> 
> 8630   // rdi == 7 is extended opcode byte
> . . .
> 
> Given that the code is simply stuffing numbers (whether supplied as
> literals or as symbolic constants) into a byte stream I think these
> comments are a help when it comes to cross-checking each specific
> assembly against the corresponding numbers declared in the Intel
> manuals. So, I don't really want to remove them. Would you prefer me to
> reverse the wording as above?

I was merely commenting on the style: the rest of the file does not have 
comments like that. The
positions of prefixes, opcode families, etc is kinda implied by the code shape.


>> === src/hotspot/cpu/x86/macroAssembler_x86.cpp
>   // prefer clwb (potentially parallel writeback without evict)
>   // otherwise prefer clflushopt (potentially parallel writeback
>   // with evict)
>   // otherwise fallback on clflush (serial writeback with evict)
> 
> In the second case the comment is redundant because the need for an
> sfence is covered by the existing comment inside the if:
> 
> // need an sfence for post flush when using clflushopt or clwb
> // otherwise no no need for any synchroniaztion

Yes, this would be good to add.


>> === src/hotspot/cpu/x86/stubGenerator_x86_64.cpp
>>
>> Is it really "cmpl" here, not "cmpb"? I think aarch64 code tests for byte.
>>
>> 2942 __ cmpl(is_pre, 0);
> 
> This is a Java boolean input. I believe that means the value will be
> loaded into c_arg0 as an int so this test ought to be adequate.

Okay.

>> === src/hotspot/share/opto/c2compiler.cpp
>>
>> Why inject new cases here, instead of at the end of switch? Saves sudden 
>> "break":
>>
>>  578 break;
>>  579   case vmIntrinsics::_writeback0:
>>  580 if (!Matcher::match_rule_supported(Op_CacheWB)) return false;
>>  581 break;
>>  582   case vmIntrinsics::_writebackPreSync0:
>>  583 if (!Matcher::match_rule_supported(Op_CacheWBPreSync)) return false;
>>  584 break;
>>  585   case vmIntrinsics::_writebackPostSync0:
>>  586 if (!Matcher::match_rule_supported(Op_CacheWBPostSync)) return 
>> false;
>>  587 break;
> 
> I placed them here so they were close to the other Unsafe intrinsics. In
> particular they precede _allocateInstance, an ordering which is also the
> case in the declarations in vmSymbols.hpp.
> 
> In what sense do you mean that an extra 'break' is saved? That would be
> true as regards the textual layout. It wouldn't affect the logic of
> folding different ranges of values into branching range tests (which is
> only determined by the numeric values of the intrinsics). If you are
> concerned about the former then I would argue that placing the values in
> declaration order seems to me to be the more important concern.

I don't think we have to follow whatever ordering mess in vmSymbols.hpp. New 
code cuts into the last
case block in that switch, which is mostly about "we know about these symbols, 
they are
falling-through to the break". Adding cases with Matcher::match_rule_supported 
seems odd there. If
anything, those new cases should be moved upwards to other

Re: RFR: 8224974: Implement JEP 352

2019-08-01 Thread Andrew Dinn

Hello Dmitry,

On 01/08/2019 05:09, Dmitry Chuyko wrote:
> Just a small comment about the tests. As you can see, some of tests in
> jdk/java/nio/channels/FileChannel check various modes, arguments, and
> expected exceptions. E.g. see MapTest, Mode and Args.
> 
> I noticed that in your changes for the new map mode there are new
> exceptions thrown in different situations. For example, when the
> required mode is not supported, or it does not fit. In particular, this
> should happen correctly on systems that do not support nvram. Have you
> considered the possibility of extending or adding tests for this behavior?
I agree that these failure cases need better test coverage and
automation. However, that is not to say they have not been tested. All
current success and failure cases can be exercised by the current mmap
test (PmemTest) if run in the correct circumstance. Unfortunately,
automatic testing is not straightforward.

1) On x86_64 where MAP_SYNC is supported test PMemTest includes
instructions to exercise a successful map from a real or emulated DAX
file system. It can also be used (and has been used) to exercise the
IOException failure path in the case where the file it tries to open
belongs to a non-DAX file system (see line 99).

Note that testing for success or failure cannot be done automatically
using the test as it currently stands. Testing for a successful map
requires mounting a real or emulated DAX file system at a known mount
point and creating a writable dir to hold the mapped file. Testing for
the IOException failure requires either setting up an equivalent non-DAX
file system mount or editing the test to open the file in a non-DAX file
system dir like, say, /tmp.

A new, separate test for the IOException failure could be automated on
x86_64 by trying to map a file located in /tmp (on the assumption that
/tmp is not mounted as a DAX file system). Of course, at present this
will only give the IOException result when MAP_SYNC is supported. Given
a suitably old Linux/x86_64, UnsupportedOperationException could also be
thrown.

2) On AArch64 where MAP_SYNC is not currently supported PmemTest clearly
cannot be used to exercise the success path. However, it can be used
(and has been used) to exercise the UnsupportedOperationException
failure path.

A check for the UnsupportedOperationException failure could be automated
on AArch64 by a new test as described above. Of course, once MAP_SYNC
support arrives in a new Linux/AArch64 it would also become for
IOException to be thrown.

So, to sum up, it would be possible to add a new, automatic test that
checks for one or other failure occurring. I am happy to add such a test
to the next webrev.

regards,

Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-07-31 Thread Dmitry Chuyko


Hi Andrew,

Just a small comment about the tests. As you can see, some of tests in 
jdk/java/nio/channels/FileChannel check various modes, arguments, and 
expected exceptions. E.g. see MapTest, Mode and Args.


I noticed that in your changes for the new map mode there are new 
exceptions thrown in different situations. For example, when the 
required mode is not supported, or it does not fit. In particular, this 
should happen correctly on systems that do not support nvram. Have you 
considered the possibility of extending or adding tests for this behavior?


-Dmitry

On 7/31/19 1:55 PM, Andrew Dinn wrote:

Hi Aleksey

On 30/07/2019 17:00, Aleksey Shipilev wrote:

On 7/30/19 5:04 PM, Andrew Dinn wrote:
...

Sure, welcome to the party even if most of the booze has been drunk :-)

Thank you for a very thorough review. Comments (mostly in agreement) are
inline below. They include a few refusals and a walk past some of the
jumps pending confirmation they truly form part of the obstacle course.

A new webrev against the latest jdk/jdk dev tree including all
uncontested changes is here:

   http://cr.openjdk.java.net/~adinn/8224974/webrev.10/


=== General:..

Re: RFR: 8224974: Implement JEP 352

2019-07-31 Thread Boris Ulasevich

Hi Andrew,

I did a quick check of the change across our platforms. Arm32 and x86_64 
built successfully. But I see it fails due to minor issues on aarch64 
and x86_32 with webrev.09.

Can you please have a look at this?

> src/hotspot/cpu/aarch64/aarch64.ad:2202:1: error: expected ‘;’ before 
‘}’ token
> src/hotspot/cpu/x86/macroAssembler_x86.cpp:9925: undefined reference 
to `Assembler::clflush(Address)'

thanks,
Boris

On 30.07.2019 18:04, Andrew Dinn wrote:

JEP 352 has now been targeted for inclusion in JDK14. The latest webrev
for the implementation JIRA has been rebased to apply to the current
tree. Is it now ok to push this change set?

JIRA:   https://bugs.openjdk.java.net/browse/JDK-8224974
webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.09

n.b. by way of sanity test I pushed this to the submit repo and it came
back with no failed tests.

regards,

Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

On 31/07/2019 16:46, Aleksey Shipilev wrote:
> On 7/31/19 4:46 PM, Andrew Dinn wrote:
>> Well, the failure happened during the build process so I didn't
>> (couldn't) debug it. I disabled the assert in supports_cpuflush() in
>> order to allow the build to complete and then ran up the resulting JVM
>> inside gdb to check what was going on. The problem is that with my patch
>> supports_cpuflush() is called from Assembler::clflush() and the latter
>> is called from icache_x86.cpp very early during bootstrap (I think this
>> is neded to flush the flush routine used to flush the code cache).
>> Anyway, the call happens so early that it precedes the call to
>> VM_Version::get_processor_features which sets up the _features mask
>> tested by the assert.
> 
> I believe you can untie this bootstrapping circularity by relaxing the assert 
> with
> Universe::is_fully_unitialized(). It still gives us window to fail with 
> SIGILL if the stub is called
> early during bootstrap, but that would be something to fix *if* we ever find 
> ourselves there.
Yes indeed, that seems like by far the cleanest solution. The only other
alternative is to move the call to VM_Version::initalize earlier in the
Universe bootstrap sequence and I doubt anyone wants to risk that.

So, now I have

#ifdef _LP64
  static bool supports_clflush() {
// clflush should always be available on x86_64
// if not we are in real trouble because we rely on it
// to flush the code cache.
// Unfortunately, Assembler::clflush is currently called as part
// of generation of the code cache flush routine. This happens
// under Universe::init before the processor features are set
// up. Assembler::flush calls this routine to check that clflush
// is allowed. So, we give the caller a free pass if Universe init
// is still in progress.
assert ((!Universe::is_fully_initialized() || (_features &
CPU_FLUSH) != 0), "clflush should be available");
return true;
  }
  . . .

which seems to work ok.

I'll post a new webrev when I have handled Brian's comments and also any
other feedback you may still have on my last reply.

regards,

Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-07-31 Thread Aleksey Shipilev

On 7/31/19 4:46 PM, Andrew Dinn wrote:
> Well, the failure happened during the build process so I didn't
> (couldn't) debug it. I disabled the assert in supports_cpuflush() in
> order to allow the build to complete and then ran up the resulting JVM
> inside gdb to check what was going on. The problem is that with my patch
> supports_cpuflush() is called from Assembler::clflush() and the latter
> is called from icache_x86.cpp very early during bootstrap (I think this
> is neded to flush the flush routine used to flush the code cache).
> Anyway, the call happens so early that it precedes the call to
> VM_Version::get_processor_features which sets up the _features mask
> tested by the assert.

I believe you can untie this bootstrapping circularity by relaxing the assert 
with
Universe::is_fully_unitialized(). It still gives us window to fail with SIGILL 
if the stub is called
early during bootstrap, but that would be something to fix *if* we ever find 
ourselves there.

-Aleksey

Re: RFR: 8224974: Implement JEP 352

On 31/07/2019 12:40, Aleksey Shipilev wrote:
> On 7/31/19 1:29 PM, Andrew Dinn wrote:
>> I have an update regarding the change to the computation of the
>> CPU_FLUSH flag. After posting the new webrev I built a debug version
>> with the change that tests the clflush bit on x86_64. It crashed when
>> the assert is reached in VM_Version::supports_cpuflush:
>>
>> # A fatal error has been detected by the Java Runtime Environment:
>> #
>> #  Internal Error (src/hotspot/cpu/x86/vm_version_x86.hpp:978),
>> pid=29597, tid=29602
>> #  assert((_features & ((uint64_t)(0x200ULL))) != 0) failed:
>> clflush should be available
>>
>> So, it seems the clflush bit is not set on my x86_64 box even though I
>> am able to execute clflush quite happily.
>>
>>   "Toto, it looks like we are no longer in Antarctica."
>>
>> So, I will revert this change (in the next webrev).
> 
> But wait, that might mean the clflush is indeed not available, by either 
> hardware or software
> switch? I believe some security mitigations disable clflush. Linux kernel, 
> for example, has
> "noclflush" option to do this. Ignoring the cpu bit and emitting clflush 
> anyway might circumvent
> that mitigation then?
Ok, so investigating further I have found out what is wrong here.
Significanlty, I get this result when I look at /proc/cpuinfo.

[root@localhost ~]# cat /proc/cpuinfo | grep flush | head -1
flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov
pat pse36 *clflush* dts acpi mmx fxsr sse sse2 ...

So, the  hardware does flag support for clflush. In which case why does
the assert in supports_clflush fail?

Well, the failure happened during the build process so I didn't
(couldn't) debug it. I disabled the assert in supports_cpuflush() in
order to allow the build to complete and then ran up the resulting JVM
inside gdb to check what was going on. The problem is that with my patch
supports_cpuflush() is called from Assembler::clflush() and the latter
is called from icache_x86.cpp very early during bootstrap (I think this
is neded to flush the flush routine used to flush the code cache).
Anyway, the call happens so early that it precedes the call to
VM_Version::get_processor_features which sets up the _features mask
tested by the assert.

So, I think the only option is to remove the assert from the x86_64
version of supports_cpuflush from Assemmbler::cpu_flush and instead add
an assert in get_processor_features that CPU_FLUSH is set on x86_64.
However, by that stage on x86_64 we will already have called clflush
and, in the case where the assert might actually fail, we may not reach
there because we have tripped up with an illegal instruction error.

regards,

Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-07-31 Thread Aleksey Shipilev

On 7/31/19 3:08 PM, Andrew Dinn wrote:
> On 31/07/2019 13:01, Boris Ulasevich wrote:
> I'm surprised by the x86_32 result as I /thought/ I had pushed the very
> same change set to the submit repo and it came back with no errors:
> 
>> Job: mach5-one-adinn-JDK-8224974-8-20190730-1325-4068436
>> BuildId: 2019-07-30-1324012.adinn.source
>> No failed tests
>   . . .
> 
> Is x86_32 tested as part of a submit run?

It does not. jdk-submit tests quite a limited number of configurations, mostly 
x86_64.

-Aleksey

Re: RFR: 8224974: Implement JEP 352

On 31/07/2019 13:01, Boris Ulasevich wrote:
> I did a quick check of the change across our platforms. Arm32 and x86_64
> built successfully. But I see it fails due to minor issues on aarch64
> and x86_32 with webrev.09.
> Can you please have a look at this?
> 
>> src/hotspot/cpu/aarch64/aarch64.ad:2202:1: error: expected ‘;’ before
> ‘}’ token
>> src/hotspot/cpu/x86/macroAssembler_x86.cpp:9925: undefined reference
> to `Assembler::clflush(Address)'
Sure, will do.

I'm surprised by the x86_32 result as I /thought/ I had pushed the very
same change set to the submit repo and it came back with no errors:

> Job: mach5-one-adinn-JDK-8224974-8-20190730-1325-4068436
> BuildId: 2019-07-30-1324012.adinn.source
> No failed tests
  . . .

Is x86_32 tested as part of a submit run?

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-07-31 Thread Aleksey Shipilev

On 7/31/19 1:29 PM, Andrew Dinn wrote:
> I have an update regarding the change to the computation of the
> CPU_FLUSH flag. After posting the new webrev I built a debug version
> with the change that tests the clflush bit on x86_64. It crashed when
> the assert is reached in VM_Version::supports_cpuflush:
> 
> # A fatal error has been detected by the Java Runtime Environment:
> #
> #  Internal Error (src/hotspot/cpu/x86/vm_version_x86.hpp:978),
> pid=29597, tid=29602
> #  assert((_features & ((uint64_t)(0x200ULL))) != 0) failed:
> clflush should be available
> 
> So, it seems the clflush bit is not set on my x86_64 box even though I
> am able to execute clflush quite happily.
> 
>   "Toto, it looks like we are no longer in Antarctica."
> 
> So, I will revert this change (in the next webrev).

But wait, that might mean the clflush is indeed not available, by either 
hardware or software
switch? I believe some security mitigations disable clflush. Linux kernel, for 
example, has
"noclflush" option to do this. Ignoring the cpu bit and emitting clflush anyway 
might circumvent
that mitigation then?

-Aleksey

Re: RFR: 8224974: Implement JEP 352

Hi Aleksey,

I have an update regarding the change to the computation of the
CPU_FLUSH flag. After posting the new webrev I built a debug version
with the change that tests the clflush bit on x86_64. It crashed when
the assert is reached in VM_Version::supports_cpuflush:

# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (src/hotspot/cpu/x86/vm_version_x86.hpp:978),
pid=29597, tid=29602
#  assert((_features & ((uint64_t)(0x200ULL))) != 0) failed:
clflush should be available

So, it seems the clflush bit is not set on my x86_64 box even though I
am able to execute clflush quite happily.

  "Toto, it looks like we are no longer in Antarctica."

So, I will revert this change (in the next webrev).

regards,


Andrew Dinn
---

On 31/07/2019 11:55, Andrew Dinn wrote:
> Hi Aleksey
> 
> On 30/07/2019 17:00, Aleksey Shipilev wrote:
>> On 7/30/19 5:04 PM, Andrew Dinn wrote:
>>> JEP 352 has now been targeted for inclusion in JDK14. The latest webrev
>>> for the implementation JIRA has been rebased to apply to the current
>>> tree. Is it now ok to push this change set?
>>>
>>> JIRA:   https://bugs.openjdk.java.net/browse/JDK-8224974
>>> webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.09
>> Is it okay to have a late code review? Here it goes:
> 
> Sure, welcome to the party even if most of the booze has been drunk :-)
> 
> Thank you for a very thorough review. Comments (mostly in agreement) are
> inline below. They include a few refusals and a walk past some of the
> jumps pending confirmation they truly form part of the obstacle course.
> 
> A new webrev against the latest jdk/jdk dev tree including all
> uncontested changes is here:
> 
>   http://cr.openjdk.java.net/~adinn/8224974/webrev.10/
> 
>> === General:
>>
>> So if pre wbsync is no-op, why do we need to handle it everywhere? We seem 
>> to be falling through all
>> the way to the stub to do nothing there, maybe we should instead cut much 
>> earlier, e.g. when
>> inlining Unsafe.writeBackPresync0? Would it be better to not emit 
>> CacheWBPreSync at all?
> 
> Ah, I see you brought your own bottle ;-)
> 
> The pre sync is definitely not needed at present. However, I put it
> there because I didn't know for sure if some future port of this
> capability (e.g. to ppc) might need to sync prior writes before writing
> back cache lines. [Indeed, the old Intel documentation stated that
> pre-sync was needed on x86 for clflush to be safe but it is definitely
> not needed.]
> 
> Anyway, folding out the pre-sync as you suggest would mean taking what
> is still potentially an architecture-specific decision in generic code.
> That may well prove to be a redundant precaution but it also costs
> little -- every affected /compile/ will last a tad longer and be a tad
> fatter in its memory footprint. If you think change is important I will
> happily oblige.
> 
>> === General:
>>
>> IIRC, notproduct and PRODUCT defines are legacy, and should be avoided? 
>> develop and ASSERT must be
>> the substitutes for this. See some discussion here: 
>> https://bugs.openjdk.java.net/browse/JDK-8183287
> 
> Ok done.
> 
> I changed #ifndef PRODUCT to #ifdef ASSERT in library_call.cpp and
> unsafe.cpp. I also redefined global flag TraceMemoryWriteback using
> develop rather than notproduct.
> 
> I also removed the AArch64 /product/ compiler option UsePOCForPOP. This
> allowed the JIT to use a memory writeback instruction with weakened
> semantics if the full writeback is not available in a product build.
> product was chosen because this was needed to establish a benchmark for
> writeback on existing kit when using a pseudo-NVRAM memory device based
> on volatile memory. The point was to be able to do an apples to apples
> comparison with writeback to a disk device. Obviously, it's not much
> use doing that test with a a non-product build. This is only important
> during development as a one-off test once the OS support for
> pseudo-NVRAM devices arrives in Linux AArch64 (which should happen well
> before new kit with the POP writeback arrives). I can still perform it
> using a custom build so I dropped this from the patch.
> 
>> === src/hotspot/cpu/aarch64/aarch64.ad
>> src/hotspot/cpu/x86/x86.ad
>>
>> This should probably be just "!VM_Version...". Braces around the statement 
>> would not hurt either.
>>
>> 2196   if (VM_Version::supports_data_cache_line_flush() == false)
>> 2197 ret_value = false;
> 
> Done.
> 
>> === src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
>> src/hotspot/cpu/x86/macroAssembler_x86.cpp
>>
>> Braces style mismatch, should be on the same line, as in the rest of the 
>> file:
>>
>> 5837 void MacroAssembler::cache_wb(Address line)
>> 5838 {
>>
>> 5856 void MacroAssembler::cache_wbsync(bool is_pre)
>> 5857 {
> 
> Done.
> 
>> === src/hotspot/cpu/x86/assembler_x86.cpp
>>
>> It feels like these comments are redundant, especially L8630 and L8646 which 
>> mention magic values
>> "6" and

Re: RFR: 8224974: Implement JEP 352

Hi Aleksey

On 30/07/2019 17:00, Aleksey Shipilev wrote:
> On 7/30/19 5:04 PM, Andrew Dinn wrote:
>> JEP 352 has now been targeted for inclusion in JDK14. The latest webrev
>> for the implementation JIRA has been rebased to apply to the current
>> tree. Is it now ok to push this change set?
>>
>> JIRA:   https://bugs.openjdk.java.net/browse/JDK-8224974
>> webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.09
> Is it okay to have a late code review? Here it goes:

Sure, welcome to the party even if most of the booze has been drunk :-)

Thank you for a very thorough review. Comments (mostly in agreement) are
inline below. They include a few refusals and a walk past some of the
jumps pending confirmation they truly form part of the obstacle course.

A new webrev against the latest jdk/jdk dev tree including all
uncontested changes is here:

  http://cr.openjdk.java.net/~adinn/8224974/webrev.10/

> === General:
> 
> So if pre wbsync is no-op, why do we need to handle it everywhere? We seem to 
> be falling through all
> the way to the stub to do nothing there, maybe we should instead cut much 
> earlier, e.g. when
> inlining Unsafe.writeBackPresync0? Would it be better to not emit 
> CacheWBPreSync at all?

Ah, I see you brought your own bottle ;-)

The pre sync is definitely not needed at present. However, I put it
there because I didn't know for sure if some future port of this
capability (e.g. to ppc) might need to sync prior writes before writing
back cache lines. [Indeed, the old Intel documentation stated that
pre-sync was needed on x86 for clflush to be safe but it is definitely
not needed.]

Anyway, folding out the pre-sync as you suggest would mean taking what
is still potentially an architecture-specific decision in generic code.
That may well prove to be a redundant precaution but it also costs
little -- every affected /compile/ will last a tad longer and be a tad
fatter in its memory footprint. If you think change is important I will
happily oblige.

> === General:
> 
> IIRC, notproduct and PRODUCT defines are legacy, and should be avoided? 
> develop and ASSERT must be
> the substitutes for this. See some discussion here: 
> https://bugs.openjdk.java.net/browse/JDK-8183287

Ok done.

I changed #ifndef PRODUCT to #ifdef ASSERT in library_call.cpp and
unsafe.cpp. I also redefined global flag TraceMemoryWriteback using
develop rather than notproduct.

I also removed the AArch64 /product/ compiler option UsePOCForPOP. This
allowed the JIT to use a memory writeback instruction with weakened
semantics if the full writeback is not available in a product build.
product was chosen because this was needed to establish a benchmark for
writeback on existing kit when using a pseudo-NVRAM memory device based
on volatile memory. The point was to be able to do an apples to apples
comparison with writeback to a disk device. Obviously, it's not much
use doing that test with a a non-product build. This is only important
during development as a one-off test once the OS support for
pseudo-NVRAM devices arrives in Linux AArch64 (which should happen well
before new kit with the POP writeback arrives). I can still perform it
using a custom build so I dropped this from the patch.

> === src/hotspot/cpu/aarch64/aarch64.ad
> src/hotspot/cpu/x86/x86.ad
> 
> This should probably be just "!VM_Version...". Braces around the statement 
> would not hurt either.
> 
> 2196   if (VM_Version::supports_data_cache_line_flush() == false)
> 2197 ret_value = false;

Done.

> === src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
> src/hotspot/cpu/x86/macroAssembler_x86.cpp
> 
> Braces style mismatch, should be on the same line, as in the rest of the file:
> 
> 5837 void MacroAssembler::cache_wb(Address line)
> 5838 {
> 
> 5856 void MacroAssembler::cache_wbsync(bool is_pre)
> 5857 {

Done.

> === src/hotspot/cpu/x86/assembler_x86.cpp
> 
> It feels like these comments are redundant, especially L8630 and L8646 which 
> mention magic values
> "6" and "7", not present in the code:
> 
> 8624   // instruction prefix is 0x66
> 
> 8627   // opcode family is 0x0f 0xAE
> 
> 8630   // extended opcode byte is 7 == rdi
> 
> 8640   // instruction prefix is 0x66
> 
> 8643   // opcode family is 0x0f 0xAE
> 
> 8646   // extended opcode byte is 6 == rsi

Well, I think the question of redundancy depends on how you look at it.
The comments are not there to explain that these values are the ones
being passed to the emit functions -- that is pretty obvious. They are
there to explain what those values mean i.e. to explain what elements of
the fully assembled instruction the emit calls assembling piecemeal.
Perhaps that reading woudl be clearer if each such equation of terms was
done in reverse order:

8624   // 0x66 is instruction prefix

8627   // 0x0f 0xAE is opcode family

8630   // rdi == 7 is extended opcode byte
. . .

Given that the code is simply stuffing numbers (whether supplied as
literals or as symbolic constants) into a

Re: RFR: 8224974: Implement JEP 352

2019-07-30 Thread Aleksey Shipilev

On 7/30/19 5:04 PM, Andrew Dinn wrote:
> JEP 352 has now been targeted for inclusion in JDK14. The latest webrev
> for the implementation JIRA has been rebased to apply to the current
> tree. Is it now ok to push this change set?
> 
> JIRA:   https://bugs.openjdk.java.net/browse/JDK-8224974
> webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.09
Is it okay to have a late code review? Here it goes:

=== General:

So if pre wbsync is no-op, why do we need to handle it everywhere? We seem to 
be falling through all
the way to the stub to do nothing there, maybe we should instead cut much 
earlier, e.g. when
inlining Unsafe.writeBackPresync0? Would it be better to not emit 
CacheWBPreSync at all?

=== General:

IIRC, notproduct and PRODUCT defines are legacy, and should be avoided? develop 
and ASSERT must be
the substitutes for this. See some discussion here: 
https://bugs.openjdk.java.net/browse/JDK-8183287

=== src/hotspot/cpu/aarch64/aarch64.ad
src/hotspot/cpu/x86/x86.ad

This should probably be just "!VM_Version...". Braces around the statement 
would not hurt either.

2196   if (VM_Version::supports_data_cache_line_flush() == false)
2197 ret_value = false;

=== src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
src/hotspot/cpu/x86/macroAssembler_x86.cpp

Braces style mismatch, should be on the same line, as in the rest of the file:

5837 void MacroAssembler::cache_wb(Address line)
5838 {

5856 void MacroAssembler::cache_wbsync(bool is_pre)
5857 {

=== src/hotspot/cpu/x86/assembler_x86.cpp

It feels like these comments are redundant, especially L8630 and L8646 which 
mention magic values
"6" and "7", not present in the code:

8624   // instruction prefix is 0x66

8627   // opcode family is 0x0f 0xAE

8630   // extended opcode byte is 7 == rdi

8640   // instruction prefix is 0x66

8643   // opcode family is 0x0f 0xAE

8646   // extended opcode byte is 6 == rsi

=== src/hotspot/cpu/x86/macroAssembler_x86.cpp

These comments feel redundant too. Well, maybe they should instead talk about 
the choices the
subsequent code makes.

9915   // pick the correct implementation

9936   // pick the correct implementation

=== src/hotspot/cpu/x86/macroAssembler_x86.cpp
   src/hotspot/cpu/x86/vm_version_x86.hpp

The docs say "The CLFLUSH instruction was introduced with the SSE2 extensions; 
however, because it
has its own CPUID feature flag, it can be implemented in IA-32 processors that 
do not include the
SSE2 extensions. Also, detecting the presence of the SSE2 extensions with the 
CPUID instruction does
not guarantee that the CLFLUSH instruction is implemented in the processor."

Yet, we have:

9910   // 64 bit cpus always support clflush
9911   assert(VM_Version::supports_clflush(), "should not reach here on 
32-bit");

 505 #ifdef _LP64
 506 // cpflush is always available on x86_64
 507 result |= CPU_FLUSH;
 508 #else
 509 if (_cpuid_info.std_cpuid1_edx.bits.clflush != 0)
 510   result |= CPU_FLUSH;
 511 #endif

 967   // 64 bit cpus always support clflush which writes back and evicts
 968   // on 32 bit cpus support is recorded via a feature flag

 980   static bool supports_clflush() { return true; }

I think the assert should just say "clflush should be available", and clflush 
cpu flag detected for
64-bits too?

=== src/hotspot/cpu/x86/macroAssembler_x86.hpp

Accidental camelCase, while hotspot code expects snake_case:

1794   void cache_wbsync(bool isPre);

=== src/hotspot/cpu/x86/stubGenerator_x86_64.cpp

Any reason to avoid inline here, e.g. "__ cache_wb(Address(src, 0))"?

2921 const Address line(src, 0);
2922 __ cache_wb(line);

=== src/hotspot/cpu/x86/stubGenerator_x86_64.cpp

Is it really "cmpl" here, not "cmpb"? I think aarch64 code tests for byte.

2942 __ cmpl(is_pre, 0);

=== src/hotspot/share/classfile/vmSymbols.hpp

Excess space before "jdk_internal..." here:

1096   do_intrinsic(_writebackPostSync0,jdk_internal_misc_Unsafe,
writebackPostSync0_name, void_method_signature , F_RN)   \

=== src/hotspot/share/opto/c2compiler.cpp

Why inject new cases here, instead of at the end of switch? Saves sudden 
"break":

 578 break;
 579   case vmIntrinsics::_writeback0:
 580 if (!Matcher::match_rule_supported(Op_CacheWB)) return false;
 581 break;
 582   case vmIntrinsics::_writebackPreSync0:
 583 if (!Matcher::match_rule_supported(Op_CacheWBPreSync)) return false;
 584 break;
 585   case vmIntrinsics::_writebackPostSync0:
 586 if (!Matcher::match_rule_supported(Op_CacheWBPostSync)) return false;
 587 break;

=== src/hotspot/share/opto/library_call.cpp

Accidental camelCase here, should be snake_case:

  257   bool inline_unsafe_writebackSync0(bool isPre);

 2870 bool LibraryCallKit::inline_unsafe_writebackSync0(bool isPre) {

=== src/hotspot/share/prims/unsafe.cpp

Odd indenting near "CC":

1122 {CC "writeback0", CC "(" "J" ")V",   
FN_PTR(Unsafe_WriteBack0)},
1123 {CC

Re: RFR: 8224974: Implement JEP 352

2019-07-30 Thread Andrew Dinn

JEP 352 has now been targeted for inclusion in JDK14. The latest webrev
for the implementation JIRA has been rebased to apply to the current
tree. Is it now ok to push this change set?

JIRA:   https://bugs.openjdk.java.net/browse/JDK-8224974
webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.09

n.b. by way of sanity test I pushed this to the submit repo and it came
back with no failed tests.

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-07-02 Thread Andrew Dinn

Hi Alan,

On 02/07/2019 16:59, Alan Bateman wrote:
> On 18/06/2019 12:43, Andrew Dinn wrote:
> One nit is that the update to the Goals section says "rework" in two
> places. I think it might more accurate to say "upgrade" or "update" as
> it doesn't significantly re-working the existing implementation.
Thanks for checking this rewording. I have updated the JEP text modulo
replacing reworked with upgraded.

regards,


Andrew Dinn
---

Re: RFR: 8224974: Implement JEP 352

2019-07-02 Thread Alan Bateman


On 18/06/2019 12:43, Andrew Dinn wrote:

Hi Alan,

Thanks for reviewing the JEP one more time.

The proposed updates look good to me.

One nit is that the update to the Goals section says "rework" in two 
places. I think it might more accurate to say "upgrade" or "update" as 
it doesn't significantly re-working the existing implementation.


-Alan

Re: RFR: 8224974: Implement JEP 352

2019-06-24 Thread Andrew Dinn

Hi Alan,

I have uploaded a webrev which I believe answers all outstanding review
comments:

  JIRA:   https://bugs.openjdk.java.net/browse/JDK-8224974
  webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.08/

This includes the changes Joe Darcy highlighted in his code review of
the implementation CSR (module header comment plus @since 14 changes in
Unsafe.java). It also allows for the tweaks to the force API
documentation (JDK-8226203).

So, I think the only work still outstanding is to agree changes to the
JEP wording (as proposed in my previous post -- see below) plus
endorsement and targeting of the JEP.

regards,


Andrew Dinn
---

On 18/06/2019 12:43, Andrew Dinn wrote:
> Hi Alan,
> 
> Thanks for reviewing the JEP one more time.
> 
> On 16/06/2019 09:47, Alan Bateman wrote:
>> I re-read the JEP, trying to put myself in the position of someone
>> reading it for the first time in 2020.
>>
>> Summary section:
>>
>> What would you think about replacing this with a sentence that makes it
>> clear that the JEP adds new JDK-specific file mapping modes to allow the
>> FileChannel API create MappedByteBuffers over non-volatile memory?
> 
> I would be happy with that way of describing the behaviour change except
> that what you suggest doesn't mention actually making it work -- which
> /is/ part of the JEP. How about:
> 
> "Add new JDK-specific file mapping modes, allowing the FileChannel API
> to be used to create MappedByteBuffers over non-volatile memory, and
> extend the underlying implementation to support this new use case"
> 
>> Goals section:
>>
>> I think the first paragraph could be re-worded to make it clear that the
>> goal is to use the existing MappedByteBuffer API to access NVM.
> 
> Yes indeed:
> 
> "This JEP proposes that class MappedByteBuffer be reworked to support
> access to non-volatile memory (NVM). The only API change required is a
> new enumeration employed by FileChannel clients to request mapping of a
> file located on an NVM-backed file system rather than a conventional,
> file storage system. Recent changes to the MappedByteBufer API mean that
> it supports all the behaviours needed to allow direct memory updates and
> provide the durability guarantees needed for higher level, Java client
> libraries to implement persistent data types (e.g. block file systems,
> journaled logs, persistent objects, etc.). The implementations of
> FileChannel and MappedByteBuffer need revising to be aware of this new
> backing type for the mapped file."
> 
>> Paragraphs 2-5 split this into two sub-goals. The first suggests that it
>> extends the MBB API but this is no longer the case. The second also
>> hints that it adds a new API. I think these two need to be re-worded.
> 
> Agreed. How about this:
> 
> "The primary goal of this JEP is to ensure that clients can access and
> update NVM from a Java program efficiently and coherently. A key element
> of this goal is to ensure that individual writes (or small groups of
> contiguous writes) to a buffer region can be committed with minimal
> overhead i.e. to ensure that any changes which might still be in cache
> are written back to memory."
> 
> n.b. I snuck in the word coherence because I thought it clarified the
> notion of committing to memory.
> 
>> Goal 3 and 4 are okay, although I think the 4th could be summarized as
>> allowing mapped buffers on NVM to be tracked by the existing monitoring
>> and management APIs.
> 
> Agreed. So, renumbering accordingly, how about this:
> 
> "A second, subordinate goal is to implement this commit behaviour using
> a restricted, JDK-internal API defined in class Unsafe, allowing it to
> be re-used by classes other than MappedByteBuffer that may need to
> commit NVM.
> 
> A final, related goal is to allow buffers mapped over NVM to be tracked
> by the existing monitoring and management APIs."
> 
>> Description section
>>
>> It might be clearer of "Proposed Java API Changes" were renamed to
>> "Proposed JDK-specific API changes".
> 
> Agreed.
> 
>> One other thing to mention is that I re-read the javadoc for the MBB
>> force methods and I think we need to adjust one of the sentences in the
>> existing (and new) methods to take account of implementation specific
>> map modes. I've created JDK-8226203 [1] to track it. As support for
>> implementation specific map modes is in new in Java SE 13 then it might
>> be worth trying to get that fixed now while it is fresh in our minds.
> Sure, I'll post an RFR with the javadoc patch as a separate step. Can it
> go into jdk13? or is it too late for that?
> 
> regards,
> 
> 
> Andrew Dinn
> ---
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-06-18 Thread Andrew Dinn

Hi Alan,

Thanks for reviewing the JEP one more time.

On 16/06/2019 09:47, Alan Bateman wrote:
> I re-read the JEP, trying to put myself in the position of someone
> reading it for the first time in 2020.
> 
> Summary section:
> 
> What would you think about replacing this with a sentence that makes it
> clear that the JEP adds new JDK-specific file mapping modes to allow the
> FileChannel API create MappedByteBuffers over non-volatile memory?

I would be happy with that way of describing the behaviour change except
that what you suggest doesn't mention actually making it work -- which
/is/ part of the JEP. How about:

"Add new JDK-specific file mapping modes, allowing the FileChannel API
to be used to create MappedByteBuffers over non-volatile memory, and
extend the underlying implementation to support this new use case"

> Goals section:
> 
> I think the first paragraph could be re-worded to make it clear that the
> goal is to use the existing MappedByteBuffer API to access NVM.

Yes indeed:

"This JEP proposes that class MappedByteBuffer be reworked to support
access to non-volatile memory (NVM). The only API change required is a
new enumeration employed by FileChannel clients to request mapping of a
file located on an NVM-backed file system rather than a conventional,
file storage system. Recent changes to the MappedByteBufer API mean that
it supports all the behaviours needed to allow direct memory updates and
provide the durability guarantees needed for higher level, Java client
libraries to implement persistent data types (e.g. block file systems,
journaled logs, persistent objects, etc.). The implementations of
FileChannel and MappedByteBuffer need revising to be aware of this new
backing type for the mapped file."

> Paragraphs 2-5 split this into two sub-goals. The first suggests that it
> extends the MBB API but this is no longer the case. The second also
> hints that it adds a new API. I think these two need to be re-worded.

Agreed. How about this:

"The primary goal of this JEP is to ensure that clients can access and
update NVM from a Java program efficiently and coherently. A key element
of this goal is to ensure that individual writes (or small groups of
contiguous writes) to a buffer region can be committed with minimal
overhead i.e. to ensure that any changes which might still be in cache
are written back to memory."

n.b. I snuck in the word coherence because I thought it clarified the
notion of committing to memory.

> Goal 3 and 4 are okay, although I think the 4th could be summarized as
> allowing mapped buffers on NVM to be tracked by the existing monitoring
> and management APIs.

Agreed. So, renumbering accordingly, how about this:

"A second, subordinate goal is to implement this commit behaviour using
a restricted, JDK-internal API defined in class Unsafe, allowing it to
be re-used by classes other than MappedByteBuffer that may need to
commit NVM.

A final, related goal is to allow buffers mapped over NVM to be tracked
by the existing monitoring and management APIs."

> Description section
> 
> It might be clearer of "Proposed Java API Changes" were renamed to
> "Proposed JDK-specific API changes".

Agreed.

> One other thing to mention is that I re-read the javadoc for the MBB
> force methods and I think we need to adjust one of the sentences in the
> existing (and new) methods to take account of implementation specific
> map modes. I've created JDK-8226203 [1] to track it. As support for
> implementation specific map modes is in new in Java SE 13 then it might
> be worth trying to get that fixed now while it is fresh in our minds.
Sure, I'll post an RFR with the javadoc patch as a separate step. Can it
go into jdk13? or is it too late for that?

regards,

Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-06-16 Thread Alan Bateman


On 10/06/2019 11:09, Andrew Dinn wrote:

:
I have updated the Proposed Java API Changes to remove the changes to
map exception signature and force region specification. They were
covered in the prior enabling JIRAs/CSRs.

So, the remaining two sections mention 1) the new module + package +
enum and 2) the bean counting changes. The new section 1 clarifies when
UnsupportedOperationException is thrown vs when IOException is thrown as
part of the explanation of what the new modes are for.

I also added a paragraph to the alternatives section explaining that
Panama was and still is an alternative option and why we decided to
proceed with this model despite it being still under consideration.

I re-read the JEP, trying to put myself in the position of someone 
reading it for the first time in 2020.


Summary section:

What would you think about replacing this with a sentence that makes it 
clear that the JEP adds new JDK-specific file mapping modes to allow the 
FileChannel API create MappedByteBuffers over non-volatile memory?


Goals section:

I think the first paragraph could be re-worded to make it clear that the 
goal is to use the existing MappedByteBuffer API to access NVM.


Paragraphs 2-5 split this into two sub-goals. The first suggests that it 
extends the MBB API but this is no longer the case. The second also 
hints that it adds a new API. I think these two need to be re-worded. 
Goal 3 and 4 are okay, although I think the 4th could be summarized as 
allowing mapped buffers on NVM to be tracked by the existing monitoring 
and management APIs.


Description section

It might be clearer of "Proposed Java API Changes" were renamed to 
"Proposed JDK-specific API changes".



One other thing to mention is that I re-read the javadoc for the MBB 
force methods and I think we need to adjust one of the sentences in the 
existing (and new) methods to take account of implementation specific 
map modes. I've created JDK-8226203 [1] to track it. As support for 
implementation specific map modes is in new in Java SE 13 then it might 
be worth trying to get that fixed now while it is fresh in our minds.


-Alan

[1] https://bugs.openjdk.java.net/browse/JDK-8226203

Re: RFR: 8224974: Implement JEP 352

2019-06-11 Thread Andrew Dinn

On 10/06/2019 18:44, Alan Bateman wrote:
> On 10/06/2019 11:09, Andrew Dinn wrote:
>> :
>> Finally, I am unclear whether the presence of the new module + package
>> means this is an SE JEP or a JDK JEP? In the latest update I have
>> assumed the JEP type is still JDK and changed the title for the public
>> API changes to "Proposed Java API Changes". Is that right? Or do you
>> want the JEP type to be SE and have the title remain "Proposed Java SE
>> API Changes"
>>
> The module is JDK-specific so keeping the scope "JDK" is right.
Ok, in that case I'll ask Brian if he can endorse the JEP and then
target for JDK14.

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-06-11 Thread Andrew Dinn

On 11/06/2019 00:30, Brian Burkhalter wrote:
> I wanted to let you know that I ran version 7 above through our tests
> and did not see any problems related to your changes.

Excellent. Thanks very much for running that check.

> I think the copyright years in PmemTest.java are wrong however: 2002,
> 2011. I suppose it should be just 2019.
Well spotted! Let's hope that is the final escapee.

I have tweaked my latest patch accordingly. I have also updated the
patch to specify @since 14 instead of @since 13 in the two places where
new elements are added. I'll post a final webrev once I am sure there
are no other changes needed.

regards,

Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-06-10 Thread Brian Burkhalter

Hi Andrew,

> On Jun 7, 2019, at 7:10 AM, Andrew Dinn  wrote:
> 
>> I have uploaded a new webrev to fix the bove problems. This version also
>> removes all the extra extraneous whitespace found by Brian and Gustavo.
>> 
>>  webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.07/
>> 
>> I have posted the changes to the submit repo to re-verify that all
>> builds pass. I have also asked Jonathan Halliday to re-test this version
>> against the Red Hat middleware clients to ensure there are no functional
>> or performance changes.
>> 
>> Modulo confirmation of those two checks this looks like it is a complete
>> implementation. Unless anyone has more changes to recommend?
> I just want to confirm that the submit job was clean and the middleware
> clients are performing as expected.

I wanted to let you know that I ran version 7 above through our tests and did 
not see any problems related to your changes.

I think the copyright years in PmemTest.java are wrong however: 2002, 2011. I 
suppose it should be just 2019.

Thanks,

Brian

Re: RFR: 8224974: Implement JEP 352

2019-06-10 Thread Alan Bateman


On 10/06/2019 11:09, Andrew Dinn wrote:

:
Finally, I am unclear whether the presence of the new module + package
means this is an SE JEP or a JDK JEP? In the latest update I have
assumed the JEP type is still JDK and changed the title for the public
API changes to "Proposed Java API Changes". Is that right? Or do you
want the JEP type to be SE and have the title remain "Proposed Java SE
API Changes"


The module is JDK-specific so keeping the scope "JDK" is right.

-Alan

Re: RFR: 8224974: Implement JEP 352

2019-06-10 Thread Andrew Dinn

Hi Alan,

On 10/06/2019 09:18, Alan Bateman wrote:
> I don't have any other feedback on the JEP beyond my last mail about
> removing the "Proposed Java API Changes" section from the Description.
> Once we you ready then Brian (as area lead) should be contacted to seek
> his endorsement.
I have updated the Proposed Java API Changes to remove the changes to
map exception signature and force region specification. They were
covered in the prior enabling JIRAs/CSRs.

So, the remaining two sections mention 1) the new module + package +
enum and 2) the bean counting changes. The new section 1 clarifies when
UnsupportedOperationException is thrown vs when IOException is thrown as
part of the explanation of what the new modes are for.

I also added a paragraph to the alternatives section explaining that
Panama was and still is an alternative option and why we decided to
proceed with this model despite it being still under consideration.

Finally, I am unclear whether the presence of the new module + package
means this is an SE JEP or a JDK JEP? In the latest update I have
assumed the JEP type is still JDK and changed the title for the public
API changes to "Proposed Java API Changes". Is that right? Or do you
want the JEP type to be SE and have the title remain "Proposed Java SE
API Changes"

regards,

Andrew Dinn
---

Re: RFR: 8224974: Implement JEP 352

2019-06-10 Thread Alan Bateman


On 10/06/2019 08:56, Andrew Dinn wrote:

:
Yes, I agree it is too late to squeeze this into 13. I have updated the
fix version for the JEP and CSR to 14:

   JEP: https://bugs.openjdk.java.net/browse/JDK-8207851
   CSR: https://bugs.openjdk.java.net/browse/JDK-8224975

I'll wait for any further feedback on the JEP and implementation.

The new release cadence is wonderful for situations like this as JDK 14 
is only 6 months after 13.


I don't have any other feedback on the JEP beyond my last mail about 
removing the "Proposed Java API Changes" section from the Description. 
Once we you ready then Brian (as area lead) should be contacted to seek 
his endorsement.


-Alan

[1] https://mail.openjdk.java.net/pipermail/nio-dev/2019-June/006238.html

Re: RFR: 8224974: Implement JEP 352

2019-06-10 Thread Andrew Dinn

On 09/06/2019 16:53, Alan Bateman wrote:
> On 07/06/2019 12:24, Andrew Dinn wrote:
>> :
>> Modulo confirmation of those two checks this looks like it is a complete
>> implementation. Unless anyone has more changes to recommend?
>>
> I read through the recent mails and I think you are nearly done with the
> code review. There are still a few updates to be done to the JEP and it
> will need to be endorsed before propose-to-target. Given that JDK 13
> enters RDP1 on June 13 then I assume it's best to re-target the CSR to
> 14 and for the JEP to become a candidate for that release.
Yes, I agree it is too late to squeeze this into 13. I have updated the
fix version for the JEP and CSR to 14:

  JEP: https://bugs.openjdk.java.net/browse/JDK-8207851
  CSR: https://bugs.openjdk.java.net/browse/JDK-8224975

I'll wait for any further feedback on the JEP and implementation.

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-06-09 Thread Alan Bateman


On 07/06/2019 12:24, Andrew Dinn wrote:

:
Modulo confirmation of those two checks this looks like it is a complete
implementation. Unless anyone has more changes to recommend?

I read through the recent mails and I think you are nearly done with the 
code review. There are still a few updates to be done to the JEP and it 
will need to be endorsed before propose-to-target. Given that JDK 13 
enters RDP1 on June 13 then I assume it's best to re-target the CSR to 
14 and for the JEP to become a candidate for that release.


-Alan.

Re: RFR: 8224974: Implement JEP 352

2019-06-07 Thread Andrew Dinn

On 07/06/2019 12:24, Andrew Dinn wrote:
> On 06/06/2019 18:40, Vladimir Kozlov wrote:
>> Hotspot changes looks good.
>>
>> CheckGraalIntrinsics failed because of typo - you should use 'J' instead
>> of 'L':
>>
>> +  "jdk/internal/misc/Unsafe.writeback0(L)V",
> 
> Doh! Thanks, Vladimir :-)
> 
> With that correction the test now passes.
> 
>> One nitpick in vmSymbols.hpp spacing is off in next line:
>>
>> +  do_intrinsic(_writebackPostSync0,   
>> jdk_internal_misc_Unsafe, writebackPostSync0_name,
>> void_method_signature , F_RN)   \
> That line and the one two lines further on were both misaligned.
> 
> I have uploaded a new webrev to fix the bove problems. This version also
> removes all the extra extraneous whitespace found by Brian and Gustavo.
> 
>   webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.07/
> 
> I have posted the changes to the submit repo to re-verify that all
> builds pass. I have also asked Jonathan Halliday to re-test this version
> against the Red Hat middleware clients to ensure there are no functional
> or performance changes.
> 
> Modulo confirmation of those two checks this looks like it is a complete
> implementation. Unless anyone has more changes to recommend?
I just want to confirm that the submit job was clean and the middleware
clients are performing as expected.

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-06-07 Thread Andrew Dinn

On 06/06/2019 18:40, Vladimir Kozlov wrote:
> Hotspot changes looks good.
> 
> CheckGraalIntrinsics failed because of typo - you should use 'J' instead
> of 'L':
> 
> +  "jdk/internal/misc/Unsafe.writeback0(L)V",

Doh! Thanks, Vladimir :-)

With that correction the test now passes.

> One nitpick in vmSymbols.hpp spacing is off in next line:
> 
> +  do_intrinsic(_writebackPostSync0,   
> jdk_internal_misc_Unsafe, writebackPostSync0_name,
> void_method_signature , F_RN)   \
That line and the one two lines further on were both misaligned.

I have uploaded a new webrev to fix the bove problems. This version also
removes all the extra extraneous whitespace found by Brian and Gustavo.

  webrev: http://cr.openjdk.java.net/~adinn/8224974/webrev.07/

I have posted the changes to the submit repo to re-verify that all
builds pass. I have also asked Jonathan Halliday to re-test this version
against the Red Hat middleware clients to ensure there are no functional
or performance changes.

Modulo confirmation of those two checks this looks like it is a complete
implementation. Unless anyone has more changes to recommend?

regards,

Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-06-06 Thread Brian Burkhalter

Hi Andrew,

> On Jun 6, 2019, at 9:24 AM, Brian Burkhalter  
> wrote:
> 
>> So, here is webrev.06 as the latest official patch:
>> 
>>  http://cr.openjdk.java.net/~adinn/8224974/webrev.06/ 
>> 

I created a delta webrev versus version 5 and this looks good modulo the 
trailing whitespace which I think you wrote you were going to fix in version 7.

> I am running it through our tests now.

All tier 1-3 tests passed on our four usual platforms.

Thanks,

Brian

Re: RFR: 8224974: Implement JEP 352

2019-06-06 Thread Vladimir Kozlov


Hotspot changes looks good.

CheckGraalIntrinsics failed because of typo - you should use 'J' instead of 'L':

+  "jdk/internal/misc/Unsafe.writeback0(L)V",

One nitpick in vmSymbols.hpp spacing is off in next line:

+  do_intrinsic(_writebackPostSync0,jdk_internal_misc_Unsafe, writebackPostSync0_name, void_method_signature 
, F_RN)   \


Thanks,
Vladimir

On 6/6/19 1:53 AM, Andrew Dinn wrote:

Hi Brian,

On 06/06/2019 01:06, Brian Burkhalter wrote:

With version 6 [1] of the patch applied, tests throw this exception:

Mapped java.lang.InternalError: java.lang.NullPointerException
at
java.base/jdk.internal.misc.ExtendedMapMode.newMapMode(ExtendedMapMode.java:58)
at
java.base/jdk.internal.misc.ExtendedMapMode.(ExtendedMapMode.java:40)
at java.base/sun.nio.ch.FileChannelImpl.map(FileChannelImpl.java:1007)

Looks like a problem in the initialization order of the static final
constants: MAP_MODE_CONSTRUCTOR needs to be initialized first so I think
the *_SYNC constants need to be after the static initializer [2].


Doh! Yes, you are right.

Sorry for adding confusion. webrev.06 was not meant to be intended for
public consumption. I uploaded it last night when I set the rebuild off
but I didn't intend to advertise it until I had retested and got your
confirmation on the suggestions I made. Anyway, thanks all the same for
reviewing it, trying to run it and also finding/fixing this issue.

I have updated webrev.06 in place and PmemTest now runs ok. I still need
to have Jonathan Halliday test the patch with our Transaction and Data
Grid software before it can be properly signed off. Another submit run
will also be needed. However, PmemTest is a good enough sanity check to
validate the changes you requested.

So, here is webrev.06 as the latest official patch:

   http://cr.openjdk.java.net/~adinn/8224974/webrev.06/

It should address

   changes requested on this list by Alan and Vladimir

   all the changes you requested in your review
   (modulo those you agreed to defer on)

   the change to correct this static init order bug

   a change to graal test CheckGraalIntrinsics to declare
   the new jdk13 intrinsics in alphanum sort order
   (this was requested offline by Alan -- without it the test
   refuses to run on jdk13)

The last of those changes still leaves one issue unanswered (although it
may not actually be an issue). Vladimir is probably best able to comment.

When I run CheckGraalIntrinsics (which is actually run indirectly by
compiler/graalunit/HotspotTest) with my patched tree it fails, claiming
that Graal does not know about Unsafe.writeback0:

   java.lang.AssertionError: missing Graal intrinsics for:
   jdk/internal/misc/Unsafe.writeback0(J)V

I am assuming this is what is expected to happen? Or perhaps the test is
supposed to be added to the problem or exclude lists?


Sorry for suggesting the change that was the proximate cause of this. :-(

No problem :-) Thanks for reviewing and checking!

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-06-06 Thread Brian Burkhalter

Hi Andrew,

> On Jun 6, 2019, at 1:53 AM, Andrew Dinn  wrote:
> 
> […]
> 
> I have updated webrev.06 in place and PmemTest now runs ok. I still need
> to have Jonathan Halliday test the patch with our Transaction and Data
> Grid software before it can be properly signed off. Another submit run
> will also be needed. However, PmemTest is a good enough sanity check to
> validate the changes you requested.
> 
> So, here is webrev.06 as the latest official patch:
> 
>  http://cr.openjdk.java.net/~adinn/8224974/webrev.06/ 
> 

I am running it through our tests now.

> It should address
> 
>  […]
> 
> The last of those changes still leaves one issue unanswered (although it
> may not actually be an issue). Vladimir is probably best able to comment.
> 
> When I run CheckGraalIntrinsics (which is actually run indirectly by
> compiler/graalunit/HotspotTest) with my patched tree it fails, claiming
> that Graal does not know about Unsafe.writeback0:
> 
>  java.lang.AssertionError: missing Graal intrinsics for:
>  jdk/internal/misc/Unsafe.writeback0(J)V
> 
> I am assuming this is what is expected to happen? Or perhaps the test is
> supposed to be added to the problem or exclude lists?

I’ll leave it to Vladimir or someone else more knowledgeable than I to comment 
on this area.

>> Sorry for suggesting the change that was the proximate cause of this. :-(
> No problem :-) Thanks for reviewing and checking!

You are welcome!

Cheers,

Brian

Re: RFR: 8224974: Implement JEP 352

2019-06-06 Thread Andrew Dinn

Hi Brian,

On 06/06/2019 01:06, Brian Burkhalter wrote:
> With version 6 [1] of the patch applied, tests throw this exception:
> 
> Mapped java.lang.InternalError: java.lang.NullPointerException
> at
> java.base/jdk.internal.misc.ExtendedMapMode.newMapMode(ExtendedMapMode.java:58)
> at
> java.base/jdk.internal.misc.ExtendedMapMode.(ExtendedMapMode.java:40)
> at java.base/sun.nio.ch.FileChannelImpl.map(FileChannelImpl.java:1007)
> 
> Looks like a problem in the initialization order of the static final
> constants: MAP_MODE_CONSTRUCTOR needs to be initialized first so I think
> the *_SYNC constants need to be after the static initializer [2].

Doh! Yes, you are right.

Sorry for adding confusion. webrev.06 was not meant to be intended for
public consumption. I uploaded it last night when I set the rebuild off
but I didn't intend to advertise it until I had retested and got your
confirmation on the suggestions I made. Anyway, thanks all the same for
reviewing it, trying to run it and also finding/fixing this issue.

I have updated webrev.06 in place and PmemTest now runs ok. I still need
to have Jonathan Halliday test the patch with our Transaction and Data
Grid software before it can be properly signed off. Another submit run
will also be needed. However, PmemTest is a good enough sanity check to
validate the changes you requested.

So, here is webrev.06 as the latest official patch:

  http://cr.openjdk.java.net/~adinn/8224974/webrev.06/

It should address

  changes requested on this list by Alan and Vladimir

  all the changes you requested in your review
  (modulo those you agreed to defer on)

  the change to correct this static init order bug

  a change to graal test CheckGraalIntrinsics to declare
  the new jdk13 intrinsics in alphanum sort order
  (this was requested offline by Alan -- without it the test
  refuses to run on jdk13)

The last of those changes still leaves one issue unanswered (although it
may not actually be an issue). Vladimir is probably best able to comment.

When I run CheckGraalIntrinsics (which is actually run indirectly by
compiler/graalunit/HotspotTest) with my patched tree it fails, claiming
that Graal does not know about Unsafe.writeback0:

  java.lang.AssertionError: missing Graal intrinsics for:
  jdk/internal/misc/Unsafe.writeback0(J)V

I am assuming this is what is expected to happen? Or perhaps the test is
supposed to be added to the problem or exclude lists?

> Sorry for suggesting the change that was the proximate cause of this. :-(
No problem :-) Thanks for reviewing and checking!

regards,

Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

Hi Andrew,

With version 6 [1] of the patch applied, tests throw this exception:

Mapped java.lang.InternalError: java.lang.NullPointerException
at 
java.base/jdk.internal.misc.ExtendedMapMode.newMapMode(ExtendedMapMode.java:58)
at 
java.base/jdk.internal.misc.ExtendedMapMode.(ExtendedMapMode.java:40)
at java.base/sun.nio.ch.FileChannelImpl.map(FileChannelImpl.java:1007)

Looks like a problem in the initialization order of the static final constants: 
MAP_MODE_CONSTRUCTOR needs to be initialized first so I think the *_SYNC 
constants need to be after the static initializer [2].

Sorry for suggesting the change that was the proximate cause of this. :-(

Thanks,

Brian

[1] http://cr.openjdk.java.net/~adinn/8224974/webrev.06/
[2] diff

--- a/src/java.base/share/classes/jdk/internal/misc/ExtendedMapMode.java
+++ b/src/java.base/share/classes/jdk/internal/misc/ExtendedMapMode.java
@@ -37,10 +37,6 @@
 
 static final MethodHandle MAP_MODE_CONSTRUCTOR;
 
-public static final MapMode READ_ONLY_SYNC = newMapMode("READ_ONLY_SYNC");
-
-public static final MapMode READ_WRITE_SYNC = 
newMapMode("READ_WRITE_SYNC");
-
 static {
 try {
 var lookup = MethodHandles.privateLookupIn(MapMode.class, 
MethodHandles.lookup());
@@ -51,6 +47,10 @@
 }
 }
 
+public static final MapMode READ_ONLY_SYNC = newMapMode("READ_ONLY_SYNC");
+
+public static final MapMode READ_WRITE_SYNC = 
newMapMode("READ_WRITE_SYNC");
+
 private static MapMode newMapMode(String name) {
 try {
 return (MapMode) MAP_MODE_CONSTRUCTOR.invoke(name);

 

> On Jun 5, 2019, at 10:47 AM, Brian Burkhalter  
> wrote:
> 
> Hi Andrew,
> 
> Please ignore the content below: I see the changes are in version 6 which I 
> had not looked through before posting my previous message.
> 
> Thanks,
> 
> Brian
> 
>> On Jun 5, 2019, at 10:25 AM, Brian Burkhalter > > wrote:
>> 
>> I suppose these changes will be in version 7. If it is likely that comments 
>> will be forthcoming from others then it might be worth waiting to 
>> incorporate any further changes in version 7.
>

Re: RFR: 8224974: Implement JEP 352

Hi Andrew,

Please ignore the content below: I see the changes are in version 6 which I had 
not looked through before posting my previous message.

Thanks,

Brian

> On Jun 5, 2019, at 10:25 AM, Brian Burkhalter  
> wrote:
> 
> I suppose these changes will be in version 7. If it is likely that comments 
> will be forthcoming from others then it might be worth waiting to incorporate 
> any further changes in version 7.

Re: RFR: 8224974: Implement JEP 352

Hi Andrew,

> On Jun 5, 2019, at 10:10 AM, Andrew Dinn  wrote:
> 
> Thanks very much for reviewing the patch. Also, commendations on your
> eagle-eyed thoroughness.

Thanks. :-)

> I have happily accepted most corrections. I have commented (inline
> below)

Likewise.

> in a few places where I think things might need to be discussed
> further. I will upload a new webrev after these points are resolved.

OK

> On 05/06/2019 15:36, Brian Burkhalter wrote:
>> I have some minor comments on webrev.05. (webrev.06 looks rather strange.)
> 
> That is the next working version being queued up -- you may well have
> seen an intermediate version while it was uploading.

Must have done as it looks OK now.

>> MappedByteBuffer.java
>> 84:“any of other modes” -> “any of the other modes”, “this” -> “This”
>> 85:nit: usually use American spelling: “behaviour” -> “behavior” (but it
>> does not matter so much here as it is not public javadoc.
>> 172:“pasing” -> “passing”
>> 175:delete “using”
>> 181:comma or semicolon before “otherwise”
> 
> All done. I even gritted my teeth and spelled behaviour 'US-wize’.

;-)

>> 959:Is there a possibility of overflow, e.g., use Math.addExact(address,
>> length) ?
> 
> I think it assumed that all OSes currently ported (certainly Linux) will
> never map a vmem region so that addition of a valid internal offset
> might overflow. If it did we would be in big trouble as regards null
> pointer checking. So, modulo input from those wiser than me in operating
> system lore, I think this is not needed.

It sounds reasonable to leave it as-is in that case.

>> 98-110:Put symbolic name definitions at head of file?
> 
> I thought these definitions would be better placed at the point where
> the defines are needed so as to make it clear why this is being done. If
> you are strongly motivated to move them to the more normal location I
> will do so.

I think that is reasonable as well so no need to move them.

>> FileChannelImpl.c (Windows)
>> 91:Use same message as at Unix version line 135?
> 
> Perhaps but I don't think they are the same error and thougt it beter to
> reinforce that with different messages. Note the different exception
> types. In the case of Windows we should never reach this point -- at
> least not until a Windows port comes along and requires the message to
> be changed (effectively removing here and replacing it with somewhere
> else). So, an InternalError is thrown. In the Unix case the error might
> conceivably happen (because the target OS mmap implementation does not
> have proper MAP_SYNC support. Hence the use of IOException.

All right, then leave it as it is.

>> ExtendedMapMode
>> 13:Constant name should be MAP_MODE_CONSTRUCTOR.
>> 13:Insert blank line after
>> 34-36:Move to before line 13.
>> 24:Move constructor to end of class.
> Yes, all good. Thanks!

Good. I suppose these changes will be in version 7. If it is likely that 
comments will be forthcoming from others then it might be worth waiting to 
incorporate any further changes in version 7.

Thanks,

Brian

Re: RFR: 8224974: Implement JEP 352

2019-06-05 Thread Andrew Dinn

Hi Brian,

Thanks very much for reviewing the patch. Also, commendations on your
eagle-eyed thoroughness.

I have happily accepted most corrections. I have commented (inline
below) in a few places where I think things might need to be discussed
further. I will upload a new webrev after these points are resolved.

On 05/06/2019 15:36, Brian Burkhalter wrote:
> I have some minor comments on webrev.05. (webrev.06 looks rather strange.)

That is the next working version being queued up -- you may well have
seen an intermediate version while it was uploading.

> Note: Hotspot changes not reviewed here.

Sure.

> General
> Check copyrights. Some headers are missing altogether, some have
> incorrect years.

Yes, all of them are now fixed.

> MappedByteBuffer.java
> 84:“any of other modes” -> “any of the other modes”, “this” -> “This”
> 85:nit: usually use American spelling: “behaviour” -> “behavior” (but it
> does not matter so much here as it is not public javadoc.
> 172:“pasing” -> “passing”
> 175:delete “using”
> 181:comma or semicolon before “otherwise”

All done. I even gritted my teeth and spelled behaviour 'US-wize'.

> 355-365: Collapse to?
> 
> Unsafe.getUnsafe().writebackMemory(address + index, length);

Sure, done. Although personally I'm 'not much into that whole brevity
thing'.

> Unsafe.java
> 924-945:Capitalize first words of sentences.

Done.

> 959:Is there a possibility of overflow, e.g., use Math.addExact(address,
> length) ?

I think it assumed that all OSes currently ported (certainly Linux) will
never map a vmem region so that addition of a valid internal offset
might overflow. If it did we would be in big trouble as regards null
pointer checking. So, modulo input from those wiser than me in operating
system lore, I think this is not needed.

> 1290:Insert blank line after.

Done.

> FileChannelImpl.c (Unix)
> 85:“||! map_sync” -> “”|| !map_sync”

Done.

> 98-110:Put symbolic name definitions at head of file?

I thought these definitions would be better placed at the point where
the defines are needed so as to make it clear why this is being done. If
you are strongly motivated to move them to the more normal location I
will do so.

> FileChannelImpl.c (Windows)
> 91:Use same message as at Unix version line 135?

Perhaps but I don't think they are the same error and thougt it beter to
reinforce that with different messages. Note the different exception
types. In the case of Windows we should never reach this point -- at
least not until a Windows port comes along and requires the message to
be changed (effectively removing here and replacing it with somewhere
else). So, an InternalError is thrown. In the Unix case the error might
conceivably happen (because the target OS mmap implementation does not
have proper MAP_SYNC support. Hence the use of IOException.

> ExtendedMapMode
> 13:Constant name should be MAP_MODE_CONSTRUCTOR.
> 13:Insert blank line after
> 34-36:Move to before line 13.
> 24:Move constructor to end of class.
Yes, all good. Thanks!

regards,

Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR: 8224974: Implement JEP 352

2019-06-05 Thread Alan Bateman


On 05/06/2019 09:43, Andrew Dinn wrote:

On 04/06/2019 11:40, Alan Bateman wrote:

On 03/06/2019 15:37, Andrew Dinn wrote:

:

The CSR and JEP have been updated accordingly

CSR:  https://bugs.openjdk.java.net/browse/JDK-8224975
JEP:  https://bugs.openjdk.java.net/browse/JDK-8207851


The specification section in the CSR was missing the module definition
so I added that. The rest looks okay so I've added myself as Reviewer so
you can finalize it.

Thanks, Alan. Done.

I see the CSR is finalized now and I'm sure Joe will look at this in the 
coming days.


I re-read the JEP and I think it needs a few edits before seeking 
Brian's endorsement.


One thing is that the scope is "JDK" but the Description still has a 
"Proposed Java SE API Changes" sub-section. The supported interfaces are 
the new jdk.nio.mapmode module and probably the name of the new buffer 
pool, everything else is describing the implementation. I think the 
"Proposal Internal JDK API changes" section is okay.


I think the "Alternatives" section should list waiting for Project 
Panama as an alternative. We've had at two discussions here about 
buffers not being the long term API for this and I think we should at 
least acknowledge that in this section. This goes partly with the 
reference in the "Risks and Assumptions" section to the the absolute 
bulk put/get methods - these methods have been added for Java SE 13.


-Alan

Re: RFR: 8224974: Implement JEP 352

To clarify, the following comments were for the _internal_ class. These along 
with a number of my other comments are just stylistic and not absolutely 
necessary to change.

Thanks,

Brian

> On Jun 5, 2019, at 7:36 AM, Brian Burkhalter  
> wrote:
> 
> ExtendedMapMode
> 13:   Constant name should be MAP_MODE_CONSTRUCTOR.
> 13:   Insert blank line after
> 34-36:Move to before line 13.
> 24:   Move constructor to end of class.

Re: RFR: 8224974: Implement JEP 352