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

[Tom Benson]

Hi,
An updated webrev with all of these changes is in 
http://cr.openjdk.java.net/~tbenson/8042668/webrev.02/  .

In my previous note I said I'd gotten rid of 
G1BiasedArray:get_index_by_address, but it was a bit premature. That is 
still needed to get from a heap address to a region index in the case 
where the corresponding HeapRegion has not yet been created.

Tom

On 6/5/2015 3:38 PM, Tom Benson wrote:
> Hi Thomas,
>
> I had said:
> > 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.
>
> I'm testing a version that I think satisfies your requests.  I did add 
> a utility method to
> HeapRegionManager, as you suggested, which accepts a MemRegion and 
> allocates
> all of the G1 regions that constitute the range.  It returns the count 
> of regions that
> required expansion, for the ergo_verbose reporting.  So that 
> eliminated one walk in
> alloc_archive_regions.
>
> The other traversals now use addr_to_region, and advance through the 
> range using
> _hrm.next_region_in_heap , which both already existed.
>
> So I was able to get rid of HeapRegionManager::addr_to_index and 
> at_or_null.
>
> I also got rid of G1BiasedArray:get_index_by_address, which was used 
> to get indices for
> setting a range using set_by_index in a loop.  Instead, I added an 
> alternate
> G1BiasedArray::set_by_address which accepts a MemRegion and sets the 
> value for
> all entries within that range.   So the loop is in the BiasedArray 
> code itself.
>
> I'll post an updated webrev when I've finished testing the changes.
> Thanks,
> Tom
>
>
> On 6/4/2015 4:09 PM, 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.
>>
>>
>> > - 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
>>
>