RFR: 8224974: Implement JEP 352

[Andrew Dinn]

Hi Vladimir,

Thanks for the follow-up and for checking the submit failure. I have
addressed all the points you raised (point by point comments are
included below). A new  webrev which passes a submit run is here:

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

n.b. This webrev also includes changes to the javadoc for
ExtendedMapMode suggested by Alan Bateman.

regards,


Andrew Dinn
-----------

On 30/05/2019 17:58, Vladimir Kozlov wrote:
> On 5/30/19 9:08 AM, Andrew Dinn wrote:

>> I have uploaded a new webrev which attempts to address the problem.
>>
>>    http://cr.openjdk.java.net/~adinn/8224974/webrev.03
> 
> I looked only on HotSpot code.

Sure, thank you.

> stubGenerator_x86_64.cpp - in generate_data_cache_writeback() next are
> not used:
> 
> +    bool optimized = VM_Version::supports_clflushopt();
> +    bool no_evict = VM_Version::supports_clwb();

Yes. I have removed them.

> vm_version_x86.hpp it should check CPUID flag in 32-bit:
> 
> +#else
> +  static bool supports_clflush() { return true; }

I have added a new feature flag CPU_FLUSH, which is set conditional on
the cpuid1 edx bit being set. The 32-bit version of supports_clflush()
test for the presence of this feature. The 64-bit version always returns
true. On 64-bit I still set the feature flag (unconditionally) even
though it is not used. I prefer the loss of 1-2 cycles to the risk of
surprising someone looking at the feature bits in a debugger.

> We don't check has_match_rule() in LibraryCallKit any more.
> In .ad files you need to add predicate to new insrtructions:
> 
>   predicate(VM_Version::supports_data_cache_line_flush());

Ok, done.

> Also add this check to Matcher::match_rule_supported() which you can use
> then in C2Compiler::is_intrinsic_supported().

Yes, this is much better. Done.

> DISABLE_UNSAFE_WRITEBACK_INTRINSIC should be checked much early may be
> together with os::supports_map_sync() when you set
> _data_cache_line_flush_size.

Doing that that would change the behaviour. If I push the check down
into the VM_Version code as you suggest then that will disable map and
writeback operations as well as the JIT intrinsic. This test was used to
allow the benefit of the intrinsic to be measured.

However, I don't think it is worth worrying about retaining this test.
It was useful during development but is probably not going to be needed
any more. So, I have removed it.

>> Unfortunately, this latest webrev still fails when uploaded to the
>> submit repo. ...
>
>  t:/workspace/open/src/java.base/windows/native/libnio/ch/FileChannelImpl.c(64):
> error C2220: warning treated as error - no 'object' file generated
>  t:/workspace/open/src/java.base/windows/native/libnio/ch/FileChannelImpl.c(64):
> warning C4029: declared formal parameter list different from definition

Ah, got it. The change to handle the isSync argument to map0 needs
propagating to the Windows native implementation.

Also, I tweaked the Windows C code to throw InternalError if mapSync is
ever passed as true. That /should never happen/ in the current
implementation. If/when this gets ported to Windows then the code that
does the throw can be replaced with case handling to call mmap with
MAP_SYNC.