RFR(S): 8215893: Add better abstraction for pinning G1 concurrent marking bitmaps.

[Thomas Schatzl]

Hi Kishor,

On 04.10.19 03:00, Kharbas, Kishor wrote:
> Hi,
> When I worked on JDK-8211425<https://bugs.openjdk.java.net/browse/JDK-8211425>, there was a request for better abstraction for pinning G1's CM bitmaps. RFE for the request is here - JDK-8215893<https://bugs.openjdk.java.net/browse/JDK-8215893>.
> 
> Here is a proposal : http://cr.openjdk.java.net/~kkharbas/8215893/webrev.00/
> 
> Here G1PageBasedVirtualSpace pins the entire reserved memory to memory during construction. The constructor takes an additional bool flag which says "does it need to pin the memory".
> If the memory is pinned, '_special' flag is set to true. I piggy back on _special flag's behavior which is to not do actual OS (un-)commits on calls to (un)commit().
> Rest of the changes is the mechanism to pass this flag from CM bitmaps creation in G1CollectedHeap all the way to G1PageBasedVirtualSpace.
> 
> Let me know if this is a good abstraction and if there is any better way.
> 
> Thanks
> Kishor
> 

Some comments:

- in the parameter lists, if the parameters are already laid out 
line-by-line, if adding a new one, please put it on a new line as well.

- this code

   if (_special) {
     if (!rs.special()) {
       commit_internal(addr_to_page_index(_low_boundary), 
addr_to_page_index(_high_boundary));
     }

in g1PageBasedVirtualSpace looks very incomprehensible.  :)

I would prefer (pending the second reviewer's comment) to either use the 
"pinned" flag here, or even better, move the necessary commit calls into 
the (now removed) HeterogeneousHeapRegionManager::initialize().

- I would just purely from feeling prefer if the "pinned" flag parameter 
would be listed after the "type" parameter in the G1RegionToSpaceMapper. 
But that's probably just me.

Also, finally one parameter per line for the declaration/definition of 
the constructor would improve readability.

Thanks,
   Thomas