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

[Kharbas, Kishor]

Hi Stefan,

Thanks for the review. Some comments inline.

New webrev : http://cr.openjdk.java.net/~kkharbas/8215893/webrev.00_to_01/

                              http://cr.openjdk.java.net/~kkharbas/8215893/webrev.01/



> 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.

>

Fixed in the new webrev.



> - 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().

>

Made it little more comprehensible. Will see what other reviewers think about moving it somewhere else.



> - 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.

>

I did it this way to logically group the parameters. MemTracker is a tracker used by the VM everywhere and does not pertain to this class as such, so I kept it in the end.



> Also, finally one parameter per line for the declaration/definition of

> the constructor would improve readability.

>

Done.

Thank you,

Kishor



> Thanks,

>    Thomas