RFR: 8224974: Implement JEP 352

[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 writebackPreSync0();

     /**
      * Serialize writeback operations relative to following memory writes.
      */
     @HotSpotIntrinsicCandidate
     private native void writebackPostSync0();


-- 
Thanks,
-Aleksey

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20190806/3211ec16/signature.asc>