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

[Thomas Schatzl]

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.

Thanks,
  Thomas

On Tue, 2015-06-09 at 22:57 +0200, Thomas Schatzl wrote:
> Hi Tom,
> 
>   I am answering this email based on my re-review of the latest changes
> at http://cr.openjdk.java.net/~tbenson/8042668/webrev.02/ because it
> covers most responses.
> 
> First, I appreciate for considering the suggestions. I would have even
> more appreciated it for re-review, if there were a diff changeset :)
> 
> On Thu, 2015-06-04 at 16:09 -0400, Tom Benson wrote:
> > 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.
> 
> 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.
> 
> E.g. there are some ideas to allow limited copying of humongous regions
> in the future to fight region-level fragmentation.
> 
> The comment now is somewhat better. I am okay with the current version
> though.
> 
> >  > - 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. :)
> 
> - extra newline in g1Allocator.cpp:174
> 
> - g1Allocator.cpp:299:  // MemRegions to the growable array provided by
> the caller.
> 
> I would prefer using "GrowableArray" the class name in the comment
> instead of "growable array" to avoid confusion
> 
> - G1ArchiveAllocator::complete_archive: could the method verify that
> "ranges" at least contains one element, and that it's not NULL?
> 
> - G1ArchiveAllocator class definition: please provide a summary what the
> class is intended to do.
> 
> - G1ArchiveAllocator::~G1ArchiveAllocator: I recommend making
> destructors virtual by default unless there is some particular concern
> (like avoiding the increase in object size). Otherwise, when subclassing
> (and adding virtual methods) the virtual is just forgotten. But the code
> is not wrong, so keep it if you want.
> 
> - G1CollectedHeap::alloc_archive_regions: please give the "commits"
> variable an unsigned type, as it is a count.
> 
> - G1CollectedHeap::end_archive_alloc_range(): is there a reason why the
> destructor of _archive_allocator is directly called instead of
> delete'ing the instance?
> 
> - g1CollectedHeap.cpp: extra newline at 1134, 3137, 3161
> 
> - it would be really great if the asserts in the verification closures
> in g1collectedheap.cpp printed the offending object address and what it
> points to. This would save you looking for the addresses in the
> debugger, and works even if something is optimized away (i.e. in
> fastdebug mode). :)
> 
> - in g1CollectedHeap.cpp: line 6385, could the reason why archive
> regions are ignored be added here? The additional condition seems
> superfluous since they are part of the old, and we should not get here
> (imo).
> 
> - g1Collectedheap.cpp: line 6444, there is a new extra space at the end
> of the line (assuming that I did not accidentally added it).
> 
> - 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)
> 
> - 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.
> 
> - G1CollectedHeap::alloc_highest_free_region() should not be public, it
> seems to purely be a helper function.
> 
> - g1CollectedHeap.hpp:741: wouild -> would
> 
> - g1MarkSweep.cpp:281/386: I would prefer if the "if" part of an "else
> if" is on the same line as the "else", not opening an unnecessary block.
> Feel free to ignore.
> 
> - 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.
> 
> - HeapRegionManager::find_highest_free(): there is a potential overflow
> for curr in the code: int's cannot hold uints (i.e. the result of
> max_length()).
> 
> - HeapRegionManager::allocate_containing_regions() uses an int to return
> the number of committed regions in commit_count. Please use size_t.
> 
> - 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?
> 
> - 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.
> 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?
> 
> >  >  - 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.
> 
> Okay.
> 
> >  >  - 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".
> 
> Okay. So why is there the change in HeapRegionSetBase::verify_region()
> to allow empty archive regions?
> 
> >  > 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) .
> > 
> 
> Thanks.
> 
> >  > - 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^)
> 
> Okay.
> 
> >  >  - 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.
> 
> Okay, good.
> 
> >  > 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.
> 
> We do not support more than uint regions in the heap, and there is
> actually a lot of code depending on that. So if you want a larger heap
> than 2^25*(2^32-1) bytes you are out of luck. :)
> 
> >  > 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.
> 
> Okay.
> 
> >  > 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.
> 
> I agree.
> 
> >  > 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.
> 
> Thanks.
> 
> >  >   - 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.
> 
> Thanks.
> 
> >  >   - 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.
> 
> It seems good to me now.
> 
> > 
> > 
> >  >  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.
> 
> It's still there, but the change may keep it.
> 
> >  >  - 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.
> 
> Okay.
> 
> >  > 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.
> 
> Okay.
> 
> >  > 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.
> 
> See above, I am not sure that the exclusion of humongous objects here is
> valid in the first place. I may overlook something though. E.g. in the
> eager reclaim code we depend on the remembered set for humongous regions
> being correct too...
> 
> >  > 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.
> 
> Okay :)
> 
> >  >   - 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.
> 
> In the other email you answered that, thanks.
> 
> Thanks,
>   Thomas
> 
>