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

[sangheon.kim at oracle.com]

Hi Kishor,

On 10/4/19 4:15 PM, Kharbas, Kishor wrote:
>
> 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/ 
> <http://cr.openjdk.java.net/~kkharbas/8215893/webrev.01/>
>
I am reviewing the patch but have a question on top of Stefan's question[1].
Why the bimap mappers are committed? I think all troubles started from 
'committing but treating as special here. Couldn't just treat the bitmap 
mappers as 'special' without commit?
If 'not committing' is doable, couldn't simply create ReservedSpace with 
'special' enabled (independent to large page setting, which is same to 
Stefan's comment)? Or add PinnedResevedSpace to force 'special enabled'.

[1]: Another thing, can you remind me why we need the bitmaps to be 
pinned but not other structures such as the card table?

+HeterogeneousHeapRegionManager::initialize() ...

+ // We commit bitmap for all regions during initialization and mark the 
bitmap space as special.
+ // This allows regions to be un-committed while concurrent-marking 
threads are accesing the bitmap concurrently.

Thanks,
Sangheon


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