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

[Tom Benson]

Hi Thomas,
Thanks for the review.   I've made many of the changes, except where noted.
I want to look a little more at options for re-factoring the code that 
walks the
heap region array.  I'll post a separate reply about that.

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


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


 > - G1Allocator::reuse_retained_old_region(): the comment talks about
 > humongous regions that need to be released. I think that it should talk
 > about pinned regions, and at the moment I cannot see a reason to keep
 > the allocation region if it has been made pinned in the meantime too.
 > This would mean that random objects could be added into pinned regions
 > while I believe it is best to specifically allocate into pinned regions,
 > at least for now.
 >
 > I.e. the check to throw away the current allocation region should use
 > is_pinned(), not is_humongous().

A region used by a G1Allocator should never be marked 'pinned' except 
when it
is humongous, so archive regions (the only other "pinned" regions) 
should never
wind up in reuse_retained_old_region.  There is an assertion just above the
comment you mentioned that the region is not an archive region.

I could change the test from is_humongous() to is_pinned(), but that 
would imply
there is support for non-humongous pinned regions in G1Allocators.


 > - g1Allocator.cpp:189: if you expect an empty region, I would prefer that
 > the corresponding getter from HeapRegion is used instead some ad-hoc
 > test (which is correct of course, but things may change for some or
 > another reason). I.e. HeapRegion::is_free().

OK.


 > - G1Allocator::record_mem_allocate():
 >   - line 219: ... insert a filler...

OK.


 >  - I think the code in line 225-231 may cause crashes/asserts.
 >
 > 225   if ((new_top > _max) ||
 > 226       ((new_top < _max) && (remainder < 
CollectedHeap::min_fill_size()))) {
 > 227     if (old_top != _max) {
 > 228       size_t fill_size = _max - old_top;
 > 229       CollectedHeap::fill_with_object(old_top, fill_size);
 > 230       _summary_bytes_used += fill_size * HeapWordSize;
 > 231     }
 >
 > i.e. consider the situation exactly when the remaining space is too 
small to
 > fit a filler object, i.e. old_top == _max - remainder
 > (where remainder == (G1CollectedHeap::min_fill_size() - 1)). In this 
case,
 > the condition in line 226 holds (new_top < _max), and the remainder 
is too
 > small to fit. So the predicate in 227 is trivially true, and we call
 > CollectedHeap::fill_with_object(). Am I overlooking something here?

I think what you're overlooking is that "remainder" is not the remaining
space in the region's current state, but what the remainder WOULD BE 
after the
requested allocation.  If the remainder will be too small, we don't do the
current allocation, and instead fill from the old_top to _max.  That 
size will
always be greater than min_fill_size.


 >  - I would prefer if the method did not preemptively allocate a new 
region,
 > but only if requested. Now it can happen that we end up with a completely
 > empty region after the last string just filled up the previous one. (If I
 > understand correctly)

That shouldn't happen. There are 2 calls to alloc_new_region - one is at the
start of archive_mem_allocate, if there is no current 
_allocation_region. The other
is in the code that decides that the current requested allocation 
doesn't fit in
the current region.  In either case, we will do an allocation in the region
immediately, so it isn't "preemptive".


 > In G1RecordingAllocator::alloc_new_region():
 >
 > - In Hotspot-code memory ranges are typically right-open, i.e. do not
 > include the last address. G1MarkSweep::mark_range_archive() neither does
 > that, nor is it documented. (Or use a base address + size, or MemRegion).

OK.  It now takes (HeapWord*, size_t) .


 > - Possibly put G1RecordingAllocator into its own files. I am not 
insisting
 > on that.

OK, I'll take you up on your offer to not insist.  8^)


 > - g1Allocator.cpp:224,228: please use pointer_delta() when calculating
 >  differences of addresses to avoid potential overflows.

OK.  I see a couple of others below that as well, and one in 
g1CollectedHeap.cpp.


 > G1RecordingAllocator::complete_recording():
 >   - the end_alignment parameter is not documented to be in bytes (and in
 > general, it would be nice to mention the requirements that are checked
 > later). Also please use size_t for alignments, just in case. (There does
 > not seem to be an overriding performance issue).

OK, it is now "size_t end_alignment_in_bytes" , and the matching change
has been made in g1CollectedHeap::end_archive_alloc_range.


 >  - check/guarantee() that end_alignment is aligned to HeapWordSize;
 >   please give actual/expected values in the error message.
 >  - line 262: please use align_pointer_up() instead of the hand-crafted
 >   (correct) formula.
 >  - line 264: please use pointer_delta()

OK to all.


 >   - CollectedHeap::fill_with_objects() will assert if the given
 >    fill_size is < min_fill_size() -> bug

Yes!  Fixed.


 >  - there is no need for the single "return;" statement at the end of
 >  the method
 >  - line 279: I would prefer if the decrement of the index using the --
 >  postfix operator on index would be put in an extra line. It is 
surprising,
 >  and there does not seem to be a particular need here.

OK to both.


 >  - I think the method still returns two memory ranges even if the
 >  space is actually contiguous. I would not expect that.

Of course we've tested this change, and that doesn't happen. You get 1 
MemRegion
for ranges that are truly contiguous.  Normally the last G1 region 
allocated will
not be full, however, and since it is below the previously allocated ones,
there will be a gap.  I considered 'filling' the gap if it is smaller than
some threshold and just returning 1, but since CDS must be prepared to
receive 2 anyway, it doesn't seem worthwhile.


 > G1RecordingAllocator::G1RecordingAllocator():
 >   - please consider initializing _bottom/_top/_max in the constructor to
 > improve the likelihood of "clean" crashes if the API is used in the 
wrong way.

OK.


 > g1BiasedArray.hpp:
 >   - the change should not add specialized methods returning data of a
 >  particular type to the G1BiasedMappedArray base class that is used only
 >  useful for a single instance.
 >  G1BiasedMappedArray is basically an abstraction of an array, and indexes
 >  should always be of type idx_t (size_t), not uint.
 >  It is very dangerous if other users start using this method and
 >  potentially get a truncated value out of it (by not looking at the
 >  signature in detail).
 >  The change should either cast the value in the user, or add its own
 >  getter (if possible).

OK. I had been swayed by region array indices being uint throughout
heapRegionManager.  Now changed to idx_t.


 > G1CollectedHeap::is_record_alloc_too_large():
 >  - I would prefer if the comments were talking about the "why" for
 >  a given statement instead of just repeating the statement. I.e.
 >  instead of "Check whether the size would be considered humongous for
 >  a minimum-sized region.", it would be much more useful to write
 >  "We do not support humongous sized strings" or similar. Actually,
 >  it would be perfectly good if the method had a single description
 >  in the header file instead of putting almost the same comment
 >  twice here and in line 926f. Same with the comment at 939, which
 >  is more about "what" is done, not why.

Yes, I agree 'why' is better here.  It's a rather long story in this case,
but I've summarized it to:
   // Allocations in archive regions cannot be of a size that would be 
considered
   // humongous even for a minimum-sized region, because G1 region 
sizes/boundaries
   // may be different at archive-restore time.

RE: line 939, I think the "what" here is useful, that the MemRegion array
will be filled in by complete_archive.  But I don't feel strongly about it,
and will remove it if you do.


 > g1CollectedHeap::fill_with_non_humongous_objects()
 >  - any reason why the increment is half the humonguous threshold
 > and not humongous threshold-1 words?

It was because I didn't want to have to worry about the increment
plus min_fill_size being humongous, for the last fill after exiting the
loop.  Using (humongous_threshold - (min_fill_size+1)) for the increment 
would
probably also work, but honestly, I don't think it matters much.


 > G1CollectedHeap::check_archive_addresses()
 >  - not sure why it uses an uint for a size specification. It seems
 > okay, but if there is no particular reason, I would prefer to use
 > size_t for sizes.

I don't recall a compelling reason.  I'll change it.


 >  - is that method actually used anywhere? Grepping the sources I
 >  could not find any use for this method.

This is used in the CDS code, so that the error of having mismatched heap
addresses can be reported separately from a general allocation failure.


 > G1CollectedHeap::end_record_alloc_range():
 >   - why is end_alignment a uint, and not a size_t? Most if not all
 > other alignments for various purposes are size_t.

OK.


 > G1CollectedHeap::alloc_archive_regions():
 >   - if the given count is zero, there is some wasted work, and we
 >  enable the slow path. Maybe guarantee that count > 0 and ranges
 >  != NULL?

I added asserts to the check/alloc/fill entries.


 >  - I think this is the method where check_archive_addresses should
 >   be called?

It's called by CDS before calling this.  However, I have added 'guarantees'
to alloc_archive_regions that check the addresses against heap boundaries
and that they are in ascending order.


 >  - I would prefer if more guarantee()'s are used in that code
 >  instead of assert. It does not seem harmful in a performance
 >  point of view, but at least gives the fuzzy feeling that we
 >  get an immediate crash for invalid input data.

The existing and new checks in alloc_archive_region are now 'guarantee.'
The same checks are in fill_archive_region, but those are left as asserts.
The reason being that in the event the archive was corrupt, there could be
a bad range that would be handed to alloc_.  But once it has succesfully
gotten through alloc_, it will be ok for fill_, unless CDS init does 
something
wrong internally. I just like to avoid needless code bloat in the 
release version.


 >   - the count parameter should be size_t imo, just because it
 >   is a size of the given array.
 >   - 994: the comment should probably read "... in ascending
 >   (start) address order..."

OK to both.


 >   - I think the code is buggy if the first region of the given
 >  memory range is region zero (which might happen). In that case,
 >  the code does not reserve region zero it seems, because line 1013
 >  increments the start_index.

Region zero can't be part of the range, in this use.  We start allocating
at the high end of heap, right after JVM init.  However, it would also
work to initialize prev_end_index to something which is not a valid index,
like the last valid index + 1 , so I've changed the code to do that.


 >   - in line 1017, I think the comment needs to be fixed: "available"
 > already has a distinct meaning in heap region management (in this
 > code). See below.

OK.


 >   - please avoid assignment and check in the condition if not
 >  necessary for some reason (the "if ((curr_region = ...) == NULL)").
 >  The current code is just hard to read, particularly because it
 >  does not save any code size, ie.
 >
 >  1020       HeapRegion* curr_region;
 >  1021       if ((curr_region = _hrm.at_or_null(curr_index)) == NULL) {
 >
 >  seems much worse to read than
 >
 >  1020       HeapRegion* curr_region = _hrm.at_or_null(curr_index);
 >  1021       if (curr_region == NULL) {
 >
 >  Since curr_region is needed in the outer scope anyway (in the
 >  else-part), I do not see a good reason to put the assignment in
 >  the if-statement.

I don't either.  I think it wound up that way through evolution of different
versions. I've changed it.


 >   - the code from 1017 to 1035 seems to be best placed as some
 > utility method in HeapRegionManager. HeapRegionManager::at_or_null()
 > leaks details about internal region organization to G1CollectedHeap.
 > It would also allow iteration over the "heap region array", which is
 > an implementation detail of HeapRegionManager (which is, what the
 > most recent huge changes to region management explicitly tried to
 > make impossible).
 >   - HeapRegionManager::addr_to_index(): the code should use the
 > HeapRegion*'s hrm_index() getter to retrieve the region's index if
 > required. The HeapRegionManager should expose as little as possible
 > details, including that the regions are consecutive. Not sure about
 > this. I mean it's nice to have, but not necessary at all.
 > Removing it would (I think) also remove the need for the ugly change
 > in G1BiasedMappedArray.

I think the re-factoring suggested here will take a little more thought.
Part of it could be done entirely in HeapRegionManager, but I think some
needs to be in G1CollectHeap.  I think this may generate more discussion,
so I'll post a separate reply.


 >  G1CollectedHeap::fill_archive_regions():
 >   - the code should check the passed ranges for minimal validity,
 > e.g. use check_archive_addresses(). That method might also check
 > for ascending addresses in the list - at least fill_archive_regions()
 > seems to miss it. Please also put such requirements in the parameter
 > description.

As noted above, I added checks.


 >  - (also for other methods): please make sure to use the exact
 >  terminology for variable names that is used elsewhere, i.e. for
 >  start/top/end/last/bottom. Several variables are called "end_XYZ",
 >  but contain the "last" address. Making the naming uniform improves
 >  readability and avoids constant lookup up of their exact meaning.
 >  Please also try to avoid new words like "base" for no apparent
 >  reason. Also, the code might be slightly less complicated (as in
 >  less characters to be parsed and understood) if "end" were used
 >  instead of "last", which would save some "+1" calculations and
 >  "<=" in comparisons (using "<" for the latter).

OK, I tried to make that more consistent.


 >  - please rename "mr" to "reserved", as this is more specific.

OK.


 >   - line 1068   // that contain the address range.  The address
 >  range actually within the : I think the "actually" is misplaced,
 >  maybe should go at the beginning of the sentence.

No, I meant that the address range which is actually represented by
the MemRegion will not be modified.  Filler objects might be added
in the G1 region that contains that address range, if necessary, to make
it parseable.


 >  - I personally would prefer if "word_size" were defined more
 >  closely to its use. I.e. in this case, there are 30 lines
 >  between definition and use.

OK.  Removed the local.


 >  - I think fill_archive_regions() has the same bug as
 > alloc_archive_regions is the first region in the memory range is
 > region zero.

Not a bug, but I made the same change.


 >   - fill_with_non_humongous_objects() should not be called if
 >  fill_size is < min_fill_size(), as it asserts in debug mode.

Since the archive allocator won't leave a space smaller than min_fill_size
at the top of an allocation region, this won't arise.


 > VerifyArchiveOopClosure::do_oop_work():
 >   - "Archive object references a non-pinned object" - isn't it that
 >  archived objects can only reference other archived objects, not
 >  just pinned? I might be wrong...
 >    - same in the comment for VerifyArchiveRegionClosure::do_object(),
 > and VerifyRegionClosure::doHeapRegion()

Yes, text strings are wrong, code is right.  Fixed.


 > G1CollectedHeap::is_obj_dead_cond():
 >   - not sure if it is performance critical, but the changed code
 >  now always evaluates the region type.

Yes, but these are only used in heap verification.  I could have
used G1MarkSweep::in_archive_range(obj) instead, but the
HeapRegion* was already at hand.


 > G1CollectedHeap::rebuild_region_sets():
 >   - the code clears the summary_used field of the recording_allocator
 > here. Where is its value recreated? I actually do not think this is
 > required: the contents of the archive regions never change anyway.
 > It should be doable to create a test that first creates a shared
 > archive, then uses it in the next VM invocation, and checks whether
 > summary bytes are okay after full gc/evacuation failure (see below)?
 >
 > G1CollectedHeap::do_collection_pause_at_safepoint(), line 4101:
 >   - why clear the used bytes for the recording allocator since my
 >  understanding is that archive regions will never be collected?

The _allocator->set_used calls at both of the above points 'inherit' the
space allocated by the ArchiveAllocator.  EG, recalculate_used walks the
regions and adds up the used size.  It seemed best to me not to perturb
the _allocator's idea of how much heap was in use, even if some of it
had originally been allocated by the ArchiveAllocator.  If the
ArchiveAllocator's total is not cleared, verification fails because the
space is double counted.

I think this is really moot at present, since the G1ArchiveAllocator
is created and and destroyed in a single safepoint, after which the JVM
exits.  But I tested scenarios where it was allowed to stay around.


 > g1CollectedHeap.cpp:
 >   - please avoid the double-spaces between sentences in comments. At
 >  least in this file, the majority of comments do not use two spaces
 >  after a full stop.
 >   - line 3368: extra spaces before the brace

OK.


 > G1CollectedHeap.hpp:
 >   - I would prefer if methods were commented separately about what
 > they do and what they expect instead of one big blob of comments
 > before a group of methods. This leads to unnecessary searching if just
 > wanting to find out about parameter usage. Also this increases the
 > likelihood if one method is changed, the developer does not update
 > the corresponding comment lots of lines above. Do not misunderstand,
 > I am fine with an introductory/summary comment about a group of methods,
 > just not that it contains the descriptions of the method signatures,
 > like in the comment in line 735 and 748. Please do not abbreviate
 > method names in comments.

OK. I personally find it more readable and more strongly connecting
the routines the way it is, but I'll change it to your preferred form.


 >  - line " 737   // end address and returns the allocated ranges as
 > an ascending array" - what is an ascending array?
 >  738   // of MemRegions.  This can be used to create and archive a heap
 >  739   // region which can be mapped at the same fixed addresses in a 
future
 >  740   // VM instance.
 >
 > I think it should read ... create and archive a set of heap regions
 > which...
 >
 > Maybe it could be clarified what a "future VM instance" is.

OK.


 > g1MarkSweep.cpp
 >   - line 61: extra space between G1ArchiveRegionMap and G1MarkSweep::
 >   - extra newlines in line 330,331

OK.


 > G1SpaceCompactClosure::doHeapRegion(): I think any pinned regions should
 > be skipped for compaction, not only archived.

I used is_archive because Humongous regions are already conditionalized 
out above,
so using "!is_pinned" could be misleading.  Same thing for 
G1AdjustPointersClosure
and G1AdjustPointersClosure.  However, I don't feel strongly about it, 
so have
changed them.


 > G1MarkSweep::enable_archive_object_check():
 >   - indentation in the call to initialize()
 >   - the cast in front of HeapRegion::Grainbytes is superfluous
 >
 >  321     _archive_region_map.set_by_index(index++, true);
 >
 > using the ++ operator within the argument seems unnecessary here, just
 > makes the statement harder to read.

OK to all.


 > G1PrepareCompactClosure::doHeapRegion():
 >   - again I think any pinned regions should be skipped here.

Answered above.


 > g1MarkSweep.hpp:
 >
 > Please add a comment to what G1ArchiveRegionMap does/is supposed to do.
 >
 >   58   // Support for 'archive' objects, to prevent objects in 
archive regions
 >   59   // from being marked by full GCs.
 >
 > "Support for XYZ" seems a bit too sparse.
 >
 > Please at least document the parameters for mark_range_archive(),
 > whether the parameters are inclusive/exclusive, etc. Or pass a
 > MemRegion, because it has defined semantics.

OK, I've bulked up the comments.  The mark_range_archive arguments are an
address and length, not 2 addresses, so I don't think 'exclusive' is really
necessary.


 > G1VerifyLiveClosure::do_oop_work()
 >   - why is the remembered set for pinned regions not checked? We do need
 > the remembered set if it gets unpinned.

This conditional was already there for Humongous regions.  For archive
regions, there is no way for them to become 'unpinned.'  If that
is a future goal, I think a follow-on project/testing is called for.


 > heapRegion.hpp:421:
 >   - please add a little comment about what pinned/archive means and the
 > difference.

OK.


 > heapRegionManager.cpp:
 >
 > HeapRegionManager::find_highest_available():
 >   - the code will never return or consider the region zero for use. I am
 > not sure this is intended.

Yes, it was not viewed as an issue, knowing the usage.  But it's easily 
changed,
and I did so.


 >   - please document that the method will automatically commit any
 > previously uncommitted regions in the header file, not in the cpp file.
 > I mean, the comment in the cpp file would be very good start (except for
 > the "loop downwards", and comments below) for the documentation of the
 > interface.
 > At least from the method name it would not have occurred to me that it
 > also commits the region. Also, "available" has a specific meaning in the
 > context of HeapRegionManager, i.e. that the region can be allocated into
 > (either empty or already allocated into).
 > Also, the documentation does not say anything about what happens when
 > there is no suitable region.

OK.  I changed the comments and renamed it find_highest_free().


 > HeapRegionManager.hpp:
 >   - HeapRegionManager::addr_to_index(): please use the HeapRegion*'s
 > hrm_index() getter to retrieve the region's index. It is an 
implementation
 > detail that the index in the _regions table of HeapRegionManager
 > corresponds to the HeapRegion's index.
 >   - HeapRegionmanager::at_or_null(uint index) const; should be removed,
 > see above.

I'll defer this to the 'how to traverse regions' message I said
I would send separately.


 >   - HeapRegionManager::find_highest_available() method: please put input
 > parameter descriptions in the header file, not in the implementation.

OK.


 > heapRegionManager.inline.hpp.
 >  - HeapRegionManager::addr_to_index(): not sure if there is a point to
 > remind you that the lower levels already do the asserting, and have two
 > levels of brackets with a more cryptic than explaining comment ("which
 > might be outside the current heap") here.

The comment was there to tell the reader that the address range is indeed
being checked for validity, a question I would expect. But I'll remove it.


 > > Performance:  The GC changes had no significant impact on SpecJBB,
 >
 > For Specjbb2005, did you check object copy/young gc times specifically?
 > They do not do a lot of GCs in the first place, so an overall performance
 > regression would require a very large performance regression in young gc
 > times to be noticed.

I only specifically checked full GC times for the reasons I mentioned, 
relying
on impact to benchmarks to show other impact.  I can look at that as 
well with
the new version.


 > I still need to go through all is_humongous() checks to see if something
 > has been missed for pinning/archiving support.

Yes, that's exactly what I did.


Thanks,
Tom