RFR: 8194312: Support parallel and concurrent JNI global handle processing

[coleen.phillimore at oracle.com]

http://cr.openjdk.java.net/~kbarrett/8194312/open.00/src/hotspot/share/gc/shared/oopStorage.hpp.html

  463 template<typename F, typename BlockPtr> // [const] Block*
  464 inline bool OopStorage::Block::iterate_impl(F f, BlockPtr block) {

Not my typical comment, but this (above) is too terse.

  489 template<typename F, typename Storage> // Storage := [const] OopStorage
  490 inline bool OopStorage::iterate_impl(F f, Storage* storage) {

This too.  Can you add a comment before each of these that they can be 
instantiated with a const OopStorage or non-const.  ie that the function 
provides a const and non-const iteration over the block list depending 
on how instantiated.  For those of us who find English easier than 
template code.

  492   typedef typename Conditional<IsConst<Storage>::value, const Block*, Block*>::type BlockPtr;

add some short comment like:
+492: // If OopStorage is const then the Block pointers are also const.

http://cr.openjdk.java.net/~kbarrett/8194312/open.00/src/hotspot/share/gc/shared/oopStorage.cpp.html

  109 #ifdef _WINDOWS
  110 #pragma warning(push)
  111 #pragma warning(disable: 4351)
  112 #endif // _WINDOWS
  113 OopStorage::Block::Block(const OopStorage* owner, void* memory) :
  114   // VS2013 warns (C4351) that elements of _data will be *correctly* default
  115   // initialized, unlike earlier versions that *incorrectly* did not do so.
  116   // Using push/disable/pop because suppress here doesn't work.

Can you move the comment to line 110 before the pragma?

  134 OopStorage::Block::~Block() {
  135 #ifdef ASSERT
  136   // Clear fields used by block_for_ptr and entry validation, which
  137   // might help catch bugs.
  138   _allocated_bitmask = 0;
  139   _owner = NULL;
  140 #endif // ASSERT
  141 }

Why not clear the fields anyway?  Seems like the expected behavior of 
the destructor.

I feel like BasicParState should be in an oopStorage.inline.hpp file to 
be included by the GCs that need it.   Runtime code only needs to 
include oopStorage.hpp and doesn't need this code.

I don't have any other comments and looks like you've addressed most of 
my preview comments that I remember.  I've looked at the runtime changes 
for JNI and they look good.   I haven't looked at the 
parallel/concurrent iterator code that GC uses, and I only skimmed the 
test which is quite significant but good to have.

This change looks great.  Looking forward to parallel/concurrent JNI 
handle garbage collection and weak pointers from the runtime.

Thanks,
Coleen

On 1/2/18 4:41 PM, Kim Barrett wrote:
> Please review this change to the implementation of JNI global and weak
> global handles, providing infrastructure for parallel and concurrent
> processing of such handles.
>
> This change introduces OopStorage, a data structure for managing
> off-heap references to objects allocated in the Java heap. JNI global
> and weak global handles are reimplemented using OopStorage, rather
> than using JNIHandleBlock. (JNI local handles continue to use
> JNIHandleBlock; the lifetime management and concurrency issues are
> very different for local vs global handles.)
>
> This change does not change any GCs to use the new parallel and
> concurrent capabilities, only laying the foundations for doing so.
>
> Other uses of OopStorage are also being considered, in the context of
> runtime latency improvements for ZGC and other collectors. The idea is
> to change some (often weak) oop references in runtime data structures
> to OopStorage-managed handles, simplifying the GC processing and
> avoiding (potentially concurrent) GC walks of runtime data structures.
>
> As a side effect, this enhancement fixes these bugs:
> 8174790: Race adding (weak) global JNI handles and determining type of handle
> 8176454: Performance: jweak references not suitable for robust cache architecture
>
> Some things not addressed by this change include:
>
> - 8194309: JNI handle allocation failure not reported correctly
> For now, the pre-existing vm_exit_out_of_memory behavior is retained.
>
> - Updating JNIHandles to use the new Access protocol.
>
> - Serviceability Agent updates for OopStorage and JNI usage.  Just
> enough has been done to allow existing SA tests to "pass".  This will
> be addressed as a followup.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8194312
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8194312/open.00/
>
> Testing:
> Mach5 hs-tier1 through hs-tier5, jdk-nightly
>