RFR: 8224974: Implement JEP 352

[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