RFR: 8224974: Implement JEP 352

Andrew Dinn adinn at redhat.com
Mon Aug 19 11:29:32 UTC 2019


Hi Alan,

Thanks for looking at the patch again. I think I have addressed all your
concerns (comments inline). Webrev and retest results at the end.

On 16/08/2019 11:39, Alan Bateman wrote:
> I think the main changes since I looked at it previously have been in
> the tests.

That's mostly it. I did also fix a problem that was breaking build of
x86_32. I also ensured that MAP_SYNC maps fail as expected on that arch.

> On non-Linux platforms, FileChannel.map should throw UOE when invoked
> with a mode map of READ_ONLY_SYNC or READ_WRITE_SYNC so I think we need
> a test for that.
> 
> MapFail seems fragile. If you add @modules java.base/jdk.internal.misc
> to the test description then it could use Unsafe::isWritebackEnabled and
> I think would simplify the test and checks. It would mean it could run
> on all platforms. Also "MapFail" is probably too general a name for a
> test in MBB because its specific to the extended map modes, not a
> general test of map failing.

I renamed the test to MapSyncFail and modified it to run without
restriction to a specific os or cpu.

I also generalized it to run twice with a boolean arg which selects mode
READ_ONLY_SYNC on the first run and READ_WRITE_SYNC on the second one.

The logic of the test is now to expect

 1) IOException if Unsafe.isWriteBackEnabled -> true
 2) UnsupportedOperationException if Unsafe.isWriteBackEnabled -> false

If the wrong exception or neither exception is thrown the test fails.

Case 1 currently only applies for x86_64.
Case 2 applies for all other architectures.

> In passing, MappedByteBuffer load/isLoaded check the fd value before
> isSync, can force() do the same? Also the @return on the private isSync
> method is very wordy and I don't think needs to duplicate the method
> description.
Sure, I have modified force() to do that check first.

Of course, that means that force(int, int) is going to repeat the same
test -- it has to because it may be called direct without going via force().

However, that's not really a problem since the compiler should elide the
repeated check.

I also shortened the text following the @return annotation as requested.

Updated webrev:

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

Testing:

Test PmemTest:
  passes as expected on x86_64 (only arch for which DAX file system is
available)
  fails to pass as expected on aarch64 and x86_32 (however, this case is
covered by the next test)

Test MapSyncFail:
  passes with expected exceptions on Linux for x86_64 (IOException),
aarch64 and x86_32 (UnsupportedOperationException).
  not tested on other arch/OS combinations (I have no access to the
necessary kit).

Red Hat MW tests:
  All still passing successfully

submit test:
  still in progress

Is it ok to push if the submit test comes back clean?

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


More information about the core-libs-dev mailing list