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

Thomas Schatzl thomas.schatzl at oracle.com
Tue Jun 2 11:19:29 UTC 2015


Hi Tom,

On Fri, 2015-05-29 at 17:30 -0400, Tom Benson wrote:
> Hi,
> Please review these changes for JDK-8042668, which constitute the GC
> support for JDK-8059092 for storing interned strings in CDS archives
> (JEP 250).  The RFR for JDK-8059092 was recently posted by Jiangli
> Zhou, and it would be best if overall comments could go to that
> thread, with GC-specific comments here.
> 
> JBS:   https://bugs.openjdk.java.net/browse/JDK-8042668
> Webrev:   http://cr.openjdk.java.net/~brutisso/8042668/webrev.00/

- 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

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

- 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().

- 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().

- G1Allocator::record_mem_allocate():
  - line 219: ... insert a filler...

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

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

 - Possibly put G1RecordingAllocator into its own files. I am not insisting
on that.

 - g1Allocator.cpp:224,228: please use pointer_delta() when calculating
differences of addresses to avoid potential overflows.

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).
  - 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()
  - CollectedHeap::fill_with_objects() will assert if the given
fill_size is < min_fill_size() -> bug
  - 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.
  - I think the method still returns two memory ranges even if the
space is actually contiguous. I would not expect that.

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.

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

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.

g1CollectedHeap::fill_with_non_humongous_objects()
 - any reason why the increment is half the humonguous threshold
and not humongous threshold-1 words?

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.
 - is that method actually used anywhere? Grepping the sources I
could not find any use for this method.

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.

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 think this is the method where check_archive_addresses should
be called?
  - 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 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..."
  - 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.
  - 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.
  - 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.

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

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.
  - (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). :)
  - please rename "mr" to "reserved", as this is more specific.
  - 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.
  - 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.
  - I think fill_archive_regions() has the same bug as
alloc_archive_regions is the first region in the memory range is
region zero.
  - fill_with_non_humongous_objects() should not be called if
fill_size is < min_fill_size(), as it asserts in debug mode.

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

G1CollectedHeap::is_obj_dead_cond():
  - not sure if it is performance critical, but the changed code
now always evaluates the region type.

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?

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

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

g1MarkSweep.cpp
  - line 61: extra space between G1ArchiveRegionMap and G1MarkSweep::
  - extra newlines in line 330,331

G1SpaceCompactClosure::doHeapRegion(): I think any pinned regions should
be skipped for compaction, not only archived.

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.

G1PrepareCompactClosure::doHeapRegion():
  - again I think any pinned regions should be skipped here.

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.

G1VerifyLiveClosure::do_oop_work()
  - why is the remembered set for pinned regions not checked? We do need
the remembered set if it gets unpinned.

heapRegion.hpp:421:
  - please add a little comment about what pinned/archive means and the
difference.

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

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::find_highest_available() method: please put input
parameter descriptions in the header file, not in the implementation.
  - HeapRegionmanager::at_or_null(uint index) const; should be removed,
see above.

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.

[...]


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

> JVM, or Dacapo benchmarks, run on x64 Linux.  However, a small (~1%)
> increase in Full GC times was seen in tests when the shared string
> support was not in use, when runs are configured to encounter them.
> When shared strings ARE in use, the impact could be as high as 5% for
> a likely worst-case.   Please see the JBS entry for a discussion.

I still need to go through all is_humongous() checks to see if something
has been missed for pinning/archiving support.

Thanks,
  Thomas




More information about the hotspot-gc-dev mailing list