RFR 8042668: GC Support for shared heap ranges in CDS (RE: JDK-8059092)

Thu Jun 11 08:19:57 UTC 2015

Hi Tom,

  (I am copying together the two recent emails and answer them as one
because it's more convenient)

On Wed, 2015-06-10 at 21:54 -0400, Tom Benson wrote: 
> Hi,
> 
> On 6/10/2015 8:23 AM, Thomas Schatzl wrote:
> 
> > Hi again, one additional note, because I just stumbled across it: the 
> > new G1CollectedHeap::fill_with_non_humongous_objects() could be 
> > replaced by using CollectedHeap::fill_with_objects() and properly 
> > setting up CollectedHeap::_filler_array_max_size for G1.
> 
> Great idea - I've made that change as well.  The updated webrev is in 
> http://cr.openjdk.java.net/~tbenson/8042668/webrev.03/ . The diffs 
> between webrev.02 and .03 are in 
> http://cr.openjdk.java.net/~tbenson/8042668/webrev.03/diff_webrev2_webrev3.txt 

We typically create complete webrevs from the latest diff (sorry for being
somewhat demanding, but it's just a lot more convenient than a raw diff :)).
E.g. when using mq, you can stash the differences to an extra revision. At the
end of the review cycle, you can merge them into a single ready for pushing.

Example revision tree:

(tip)
base-rev

You create the webrev for base-rev by doing "webrev.sh -r -2 -u <username>"
and make it available. During review some changes were suggested - you then
add your changes, creating a new mq changeset, looking like this:

(tip)
base-rev
thomas-suggestions

The webrev for the latest changes can then be easily created doing a
"webrev.sh -r -2 -u <username>", and the full webrev e.g. by
"webrev.sh -r -3 -u <username>". Make both webrevs available. Continue
as required :)

But already providing the diff is a big time-saver.

On Wed, 2015-06-10 at 21:46 -0400, Tom Benson wrote:
> Hi Thomas,
> 
> On 6/9/2015 4:57 PM, Thomas Schatzl wrote:
>
> > On Thu, 2015-06-04 at 16:09 -0400, Tom Benson wrote:
> >>
> >>   > - Comments about that humongous regions are never added to the collection
> >>   > set in collectionSetChooser.hpp need to be updated. While it is still
> >>   > true, that humongous regions are never added, we actually never want to
> >>   > add pinned regions, which are a superset.
> >>   > E.g. collectionsetChooser.hpp:106
> >>
> >> This comment was, in fact, updated to include archive regions, but perhaps
> >> the parenthetical note made it hard to notice.  The review version says:
> >>
> >>    106   // not. Currently, we skip humongous regions (we never add them to
> >>    107   // the CSet, we only reclaim them during cleanup) and archive
> >> regions,
> >>    108   // which are both "pinned", and regions whose live bytes are
> >> over the
> >>    109   // threshold.
> >>
> >> So, I've now re-ordered it to make it more obvious.
> > The main part, and that is what the code does, is that we do not add
> > pinned regions. It is incidental that all humongous regions and archived
> > regions are pinned at the moment. Not the other way around; in that
> > sentence the the actual relevant information for deciding to add a
> > region is in a sub-clause.
> 
> OK, I've changed it to just say 'pinned regions.'
> 
> >>   > - In general it would be nice if the asserts gave more information than
> >>   > just some "this failed" message. E.g. it would be nice to at least
> >>   > print the
> >>   > index of the HeapRegion or the base address to be able to find out more
> >>   > information about it just using the hs_err file.
> >>   >
> >>   > All changed asserts have that problem, so I am not mentioning them
> >>   > explicitly.
> >>
> >> OK.  I focused on the ones I have added, rather than existing ones where
> >> I may
> >> have changed "is_humongous" to "is_pinned", for example.  But I did
> >> change the
> >> existing one in g1EvacFailure where the previous message was just "sanity".
> >>
> >> I had already changed some in g1Allocator at Bengt's suggestion since
> >> the version
> >> you reviewed.  Now I've changed others where I think it makes sense.
> > If others think this is okay, then I won't object to it any more. To me,
> > having as much as possible useful information about what is wrong in the
> > assert message seems very useful.
> >
> > E.g. with the existing comment in the first assert in
> > G1CollectedHeap::check_archive_addresses() (and other similar cases) I
> > would not be able to rule out one or the other condition failing.
> >
> > As for the part about not fixing existing bad comments, we also agree to
> > disagree that this is not useful. :)
> 
> I would always change a "bad" comment.  I think we're talking about 
> making assertions more verbose.
> But I have gone ahead and added additional info to all the asserts whose 
> conditions I touched,
> except for one, which would have required more new code to get the value 
> to print.  And I made
> the new ones more verbose, where I hadn't already.

:) Great.

[...]
> > - g1Collectedheap.cpp: line 6444, there is a new extra space at the end
> > of the line (assuming that I did not accidentally added it).
> There was one at 6445 (the comment).  I didn't add it, either.  But as 
> long as I'm fixing it,
> I corrected the grammar of the comment.

Note that JPRT would fail pushing because of that extra space. Using the
mercurial jcheck extension you can find these things automatically.

> > - g1Collectedheap.cpp: line 6450-6455: why is it required to not set
> > archive regions old? Apart from that they are already old, setting them
> > old again does not seem to hurt. (It has been done that way before)
> 
> Setting them old would clear the archive tag.   set_old does not 'or'-in 
> the 'old' tag, it just overwrites _tag.
> It seemed safer to change this test than set_old.

Okay.

> > - g1CollectedHeap.cpp: line 6639: the code specifically calls out
> > (asserts on) non-pinned regions, while the next line is a
> > ShouldNotReachHere(). It seems to be some debugging code leftover -
> > otherwise I would prefer to print the kind of region at this point
> > unconditionally.
> 
> The ShouldNotReachHere was there already, because all region types were 
> handled above.  I could have
> left it at that, because Pinned regions which are not Old or Humongous 
> should also not exist, but I added the
> assert to add the specific info in that case.    There is no set_pinned 
> type setter, only set_archive and
> set_humongous, so this verification code seems a reasonable place to 
> ensure pinned isn't set alone.
> There is not currently a way to get the _type field as raw bits to 
> print.  Could be done, of course.
> I've added this comment to possibly clarify:
> 
>        // There are no other valid region types. Check for one invalid
>        // one we can identify: pinned without old or humongous set.
>        assert(!hr->is_pinned(), err_msg("Heap region %u is pinned but 
> not old (archive) or humongous.", hr->hrm_index()));
>        ShouldNotReachHere();
> 
> 

Okay. I would have opted for just removing it.

> > - heapRegion.cpp:723: I am not sure that excluding pinned regions here
> > is what is expected. This code verifies that the to-region has a correct
> > remembered set entry. As far as I know, at the moment we still manage
> > remembered sets for the pinned (and archived) regions. It seems to me
> > that also the original code is wrong. I may be overlooking something.
> 
> My take on the exclusion of humongous regions was that it was because 
> contained objects
> would not be moved, and archive regions have the same attribute. So 
> is_humongous became
> is_pinned.  I agree that if the original code was wrong, the new code 
> is, too -  but if so, I'd be
> surprised it hadn't failed before now. I'll defer to your expertise on 
> remembered sets.   Let
> me know if you want me to change this

The original code is "wrong" because for some reason it skips verifying
the remembered set of humongous regions. I agree however on not changing
this and not worry about this at this point. I will look at what this
means.

> > - HeapRegionManager::allocate_containing_regions: I am somewhat
> > concerned about the use of uints to hold size_t arguments, ie.
> >
> >    uint start_index = _regions.get_index_by_address(range.start());
> >    uint last_index = _regions.get_index_by_address(range.last());
> >
> > Does no compiler complain about this?
> 
> Yes, there are missing casts there -  I overlooked the warning (on 
> windows) due to another build issue.
> I assume you're not talking about using uint for region indices in 
> general, because HeapRegionManager
> does it everywhere.

No, no, just the missing cast in front of the _regions.get_index_b...
call.

The decision to use uints (and only allow 2^32-2 regions) is assumed
everywhere, so it has not been changed when introducing
HeapRegionManager.

> > - why is the "|| hr->is_archive()" clause needed in
> > HeapRegionManager::verify()? Imo it would be preferable (but just
> > minimally so), like for humongous starts regions to use the orig_end()
> > member for prev_end (i.e. add to the "if (starts_humongous..." in line
> > 498).
> > The current code basically tries to verify that between regions there
> > are no holes, i.e. the heap is contiguous.
> > Now archive regions potentially (i.e. the last one) have their end() not
> > at the end of the complete region, so I understand why this would fail
> > without specially handling them.
> 
> Actually, archive regions do always have their end() at the end of the 
> complete region,
> just like normal regions.  I should remove the is_archive test.  The 
> only way I can think
> of that it got there was from an experimental version which did 
> allocation within the
> archive regions in a different way, but that should not have leaked into 
> this directory.
> Hmm.  Removed it.

Thanks.

> > As mentioned, I would prefer to extend the existing workaround for
> > humongous starts regions, instead of disabling the assert.
> >
> > - HeapRegionSetBase::verify_region(): I thought below you mentioned that
> > there can be no completely empty/free region, so why could this fail
> > with archive regions in the mix?
> 
> Yes, this can be removed.  Good catch.

Looks good. Thanks for your patience with me :)

Thanks,
  Thomas