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

sangheon.kim at oracle.com sangheon.kim at oracle.com
Wed Oct 16 18:02:50 UTC 2019


Hi Kishor,

Before reviewing webrev.02, could you remind us what was the motivation 
of pinning the bitmap mappers here?
In addition to explanations of the problematic situation, any logs / 
stack-trace also may help.

We think that understanding of the root cause should be considered first.

Thanks,
Sangheon


On 10/15/19 6:23 PM, Kharbas, Kishor wrote:
>
> Thank you for the suggestions.
>
> In this webrev I added a flag to ReservedSpace constructors to direct 
> it to pin the memory space. So now G1PageBasedVirtualSpace does not 
> have to do special handling.
>
> http://cr.openjdk.java.net/~kkharbas/8215893/webrev.02/
>
> To add more to Sangheon’s reply to Stefan’s question,
>
> > Another thing, can you remind me why we need the bitmaps to be pinned but not other structures such as the 
> card table?
>
> When I implemented this feature I had run into issue with the default 
> implementation of concurrent marking bitmaps.
>
> Thanks,
>
> Kishor
>
> *From:*sangheon.kim at oracle.com [mailto:sangheon.kim at oracle.com]
> *Sent:* Wednesday, October 9, 2019 2:42 PM
> *To:* Kharbas, Kishor <kishor.kharbas at intel.com>; 
> hotspot-gc-dev at openjdk.java.net
> *Cc:* Stefan Johansson <stefan.johansson at oracle.com>
> *Subject:* Re: RFR(S): 8215893: Add better abstraction for pinning G1 
> concurrent marking bitmaps.
>
> 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>
>     <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>
>     <https://bugs.openjdk.java.net/browse/JDK-8215893>.
>
>     >>
>
>     >> Here is a proposal :
>     http://cr.openjdk.java.net/~kkharbas/8215893/webrev.00/
>     <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
>




More information about the hotspot-gc-dev mailing list