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

Tom Benson tom.benson at oracle.com
Thu Jun 11 12:13:17 UTC 2015


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



More information about the hotspot-gc-dev mailing list