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

Jiangli Zhou jiangli.zhou at oracle.com
Mon Jun 8 20:33:32 UTC 2015


Hi Tom,

I’m integrating your new patch.

Thanks,
Jiangli

On Jun 8, 2015, at 12:45 PM, Tom Benson <tom.benson at oracle.com> wrote:

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



More information about the hotspot-gc-dev mailing list