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

[Jiangli Zhou]

Hi Tom,

On Jun 11, 2015, at 5:13 AM, Tom Benson <tom.benson at oracle.com> wrote:

> Hi,
> > We typically create complete webrevs from the latest diff
> OK, thanks.  I'll do so next time.
> 
> > Looks good. Thanks for your patience with me :)
> Thanks.   If one of the other reviewers of the earlier version can look at the update, it would be great.  I'll also need a commit sponsor.

I can help you commit the changeset if you haven’t found one yet. I see your name is still not listed as “author” in openjdk. You can request that.

For integrating the changes, we can integrate the GC and runtime changes together as one ‘push’. I can do that.

Thank you for all the work on the shared string GC support!!

Jiangli

> 
> I'll try to create a webrev with diffs between the first version and this one, if that will help.
> Tom
> 
> On 6/11/2015 4:19 AM, Thomas Schatzl wrote:
>> 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
>> 
>> 
>