RFR (M): 8201492: Properly implement non-contiguous generations for reference discovery
Thomas Schatzl
thomas.schatzl at oracle.com
Sun Apr 29 18:45:06 UTC 2018
Hi,
On Fri, 2018-04-27 at 21:23 -0400, Kim Barrett wrote:
> > On Apr 18, 2018, at 2:52 AM, Thomas Schatzl <thomas.schatzl at oracle.
> > com> wrote:
> >
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8201492
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8201492/webrev/
> > Testing:
> > hs-tier1-3, performance verification, lots of Kitchensink reference
> > stress testing runs
> >
>
> StefanJ said:
> The only thing I can comment on is the introduction of
> SpanReferenceProcessor. I think it would be nicer to just have a
> single ReferenceProcessor that always take a closure for deciding
> if
> discovery should be done, ie. let the other GCs explicitly create
> their SpanBasedDiscoverer and pass it in.
>
> I had the same reaction. It does provide [set_]span(); is that
> enough
> to make it worthwhile? Maybe?
>
> If that were changed, it may impact other comments below.
Done.
>
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/shared/referenceProcessor.cpp
> 135 SpanReferenceProcessor::SpanReferenceProcessor(MemRegion span,
> ...
> 142 ReferenceProcessor(&_span_based_discoverer,
> ...
> 149 _span_based_discoverer(span) {
>
> Passing a pointer to a member before the member has been constructed
> can lead to problems, though I didn't find any issues here. However,
> I've seen compiler warnings at some warning levels for something like
> this. (Obviously not with the levels we're testing with.)
>
> Obviously, this isn't a problem if there isn't a
> SpanReferenceProcessor.
>
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/shared/referenceProcessor.cpp
> 977 template <class T>
> 978 bool ReferenceProcessor::is_subject_to_discovery(T const obj)
> const {
> 979 return _is_subject_to_discovery->do_object_b(obj);
> 980 }
>
> I don't understand why this is a template. T *must* be oop (or at
> least convertible to oop), since do_object_b takes an oop argument.
Fixed.
>
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1ConcurrentMark.inline.hpp
> 41 inline bool G1CMIsAliveClosure::do_object_b(oop obj) {
> 42 if (obj == NULL) {
> 43 return false;
> 44 }
> 45 assert(_g1h->is_in_reserved(obj), "Asked for liveness of oop "
> PTR_FORMAT " outside of reserved heap.", p2i(obj));
> 46 // Young regions have nTAMS == bottom(), i.e. all objects
> there are implicitly live,
> 47 // so we do not need to explicitly check for region type.
> 48 bool result = !_g1h->is_obj_ill(obj, _g1h-
> >heap_region_containing(obj));
>
> I think the code in lines 42-48 is equivalent to
>
> bool result = !_g1h->is_obj_ill(obj);
>
It is. However the assert error message would not be that descriptive
(imho). I changed it to the suggested
42 return !_g1h->is_obj_ill(obj);
though.
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1ConcurrentMark.inline.hpp
> 55 inline bool G1CMSubjectToDiscoveryClosure::do_object_b(oop obj)
> {
> 56 if (obj == NULL) {
> 57 return false;
> 58 }
>
> I haven't checked carefully, but it seems wrong to check for
> discoverability of NULL; I would have expected NULL to already have
> been filtered out before getting to such a check. In which case the
> check here ought to be an assert != NULL.
>
> Oh, but when the policy is ReferentBasedDiscovery we apply this
> closure to the referent, and for a concurrent collector the referent
> could have been cleared between the time the referent was determined
> to be (potentially) not alive and when this check is made. That's
> pretty subtle, and seems worth a comment.
Added.
Webrevs
http://cr.openjdk.java.net/~tschatzl/8201492/webrev.1 (full)
http://cr.openjdk.java.net/~tschatzl/8201492/webrev.0_to_1 (diff)
I pushed the changes through hs-tier1-3 again.
Note that the diff webrev is not very helpful in this case.
Thanks,
Thomas
More information about the hotspot-gc-dev
mailing list