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