RFR 8042668: GC Support for shared heap ranges in CDS (RE: JDK-8059092)
Thomas Schatzl
thomas.schatzl at oracle.com
Tue Jun 9 20:57:41 UTC 2015
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
More information about the hotspot-runtime-dev
mailing list