Request for code review - 8130265: gctests/LargeObjects/large001 fails with OutOfMemoryError: Java heap space
Thomas Schatzl
thomas.schatzl at oracle.com
Wed Aug 26 10:26:54 UTC 2015
Hi,
looking at webrev.05, but answering here.
On Tue, 2015-08-25 at 15:25 +0200, Jesper Wilhelmsson wrote:
> Hi Alex,
>
> I like this approach :)
>
> Just one comment:
>
> After GC in the helper you put the assert that soft references should have been
> handled. The first call to this helper will not clear soft references. The
> original code only has this assert at the end of the function. I think it is
Whatever the original code did, there is the general requirement that
after any full gc, collector_policy()->should_clear_all_soft_refs()
should be false.
> enough to keep it there, at the end of satisfy_failed_allocation().
It seems to be randomly placed now, checking only after everything else
fails. I do agree that the original code did the same, but why not
improve upon this?
If you want in a separate CR.
> 1764 assert(!collector_policy()->should_clear_all_soft_refs(),
> 1765 "Flag should have been handled and cleared prior to this point");
>
> Besides that I'm fine with this version.
- Some braces next to boolean parameters are not aligned properly. E.g.
in line 1777 and 1804.
>
>
> Den 24/8/15 kl. 23:24, skrev Alexander Harlap:
> > Hi Thomas and Jesper,
> >
> > Here is new version of change:
> >
> > http://cr.openjdk.java.net/~aharlap/8130265/webrev.04/
> >
> > I followed Jesper's advice to have
> > satisfy_failed_allocation_helper(..) :
> > attempt_allocation_at_safepoint(...);
> > expand_and_allocate(...);
> > do_collection(...);
> >
> > So I did not reorder calls to attempt_allocation_at_safepoint(...) and
> > expand_and_allocate(...)
> >
> > -Alex
> >
> >
> > On 8/24/2015 8:55 AM, Thomas Schatzl wrote:
> >> Hi,
> >>
> >> On Fri, 2015-08-21 at 11:44 -0400, Alexander Harlap wrote:
> >>>
> >>> On 8/21/2015 8:18 AM, Jesper Wilhelmsson wrote:
> >>>
> >>>> Hi Alex,
> >>>>
> >>>> Alexander Harlap skrev den 20/8/15 21:41:
> >>>>> Kim and Thomas,
> >>>>>
> >>>>> Here is new version with your suggestions in:
> >>>>>
> >>>>> http://cr.openjdk.java.net/~aharlap/8130265/webrev.03/
> >>>>> <http://cr.openjdk.java.net/%7Eaharlap/8130265/webrev.03/>
> >>>>> Alex
> >>>> A few comments on this latest version.
> >>>>
> >>>> * Why do you assert_at_safepoint and set succeeded to true in your
> >>>> helper? The original code did not do this at these places. I think
> >>>> you can remove these lines.
> >>>>
> >>>> 1736 assert_at_safepoint(true /* should_be_vm_thread */);
> >>>> 1737
> >>>> 1738 *succeeded = true;
> >>>>
> >>> Even method called satisfy_failed_allocation_helper , there is no way
> >>> to make sure that it is called only from satisfy_failed_allocation.
> >> I do not think we guarantee that for any method, i.e. that a given
> >> method somehow checks its call chain. This would require much better
> >> encapsulation, and in some cases this does not help you either. Also it
> >> would require you to state the allowed callers somehow.
> >>
> >> The helper may be private too, not protected.
> >>
> >>> satisfy_failed_allocation_helper is a separate method and those lines
> >>> are needed.
> >>> So I would like to keep
> >>> 1736 assert_at_safepoint(true /* should_be_vm_thread */);
> >>> 1737
> >>> 1738 *succeeded = true;
> >>>> * Please align the arguments in
> >>>>
> >>>> 1732 G1CollectedHeap::satisfy_failed_allocation_helper(size_t
> >>>> word_size,
> >>>> 1733 AllocationContext_t
> >>>> context,
> >>>> 1734 bool
> >>>> clear_all_soft_refs,
> >>>> 1735 bool* succeeded) {
> >>>>
> >>>> and
> >>>>
> >>>> 1748 HeapWord* result =
> >>>> attempt_allocation_at_safepoint(word_size,
> >>>> 1749 context,
> >>>> 1750 true /*
> >>>> expect_null_mutator_alloc_region */);
> >>>>
> >>>> and also in the header file.
> >> Please also avoid method definitions like
> >>
> >> <return value>
> >> <class name>::<method-name> ...
> >>
> >> (or even worse
> >>
> >> <return value>
> >> <class-name>::
> >> <method-name>
> >>
> >> (this change does not do it that way)
> >> )
> >>> Sure.
> >>>
> >>>> * Your last version did:
> >>>>
> >>>> attempt_allocation_at_safepoint(word_size, context, false);
> >>>> expand_and_allocate(word_size, context);
> >>>> do_collection(false, false, word_size);
> >>>> attempt_allocation_at_safepoint(word_size, context, true);
> >>>> expand_and_allocate(word_size, context);
> >>>> do_collection(false, true, word_size);
> >>>> attempt_allocation_at_safepoint(word_size, context, true);
> >>>> assert(!collector_policy()->should_clear_all_soft_refs());
> >>>> expand_and_allocate(word_size, context);
> >>>>
> >>>> You have chosen to break this into:
> >>>>
> >>>> attempt_allocation_at_safepoint(word_size, context, false);
> >>>> expand_and_allocate(word_size, context);
> >>>>
> >>>> satisfy_failed_allocation_helper(word_size, context, false,
> >>>> succeeded);
> >>>> do_collection(false, false, word_size);
> >>>> attempt_allocation_at_safepoint(word_size, context, true);
> >>>> expand_and_allocate(word_size, context);
> >>>>
> >>>> satisfy_failed_allocation_helper(word_size, context, true,
> >>>> succeeded);
> >>>> do_collection(false, true, word_size);
> >>>> attempt_allocation_at_safepoint(word_size, context, true);
> >>>> expand_and_allocate(word_size, context);
> >>>>
> >>>> assert(!collector_policy()->should_clear_all_soft_refs());
> >>>>
> >>>> This is not entirely identical, depending on how important the
> >>>> assert is that could be an issue since the assert is now skipped in
> >>>> some cases where it was previously used.
> >> The assert just states that after setting clear_all_soft_refs to true in
> >> the call to do_collection, it must be false after collection.
> >>
> >> So actually one could change the assert to
> >>
> >> assert(!clear_all_soft_refs || !
> >> collector_policy()->should_clear_all_soft_refs(), "...") and put it
> >> directly in the helper.
> >>
> >> Actually I think CollectorPolicy::clear_all_soft_refs() must be false
> >> after any full gc, so it should be placed after the do_collection call.
> >>>> My suggestion would be to break the code up like this:
> >>>>
> >>>> satisfy_failed_allocation_helper(word_size, context, do_gc,
> >>>> clear_softrefs)
> >>>> attempt_allocation_at_safepoint(word_size, context, false);
> >>>> expand_and_allocate(word_size, context);
> >>>> do_collection(false, false, word_size);
> >>>>
> >>>> satisfy_failed_allocation_helper(word_size, context, do_gc,
> >>>> clear_softrefs)
> >>>> attempt_allocation_at_safepoint(word_size, context, true);
> >>>> expand_and_allocate(word_size, context);
> >>>> do_collection(false, true, word_size);
> >>>>
> >>>> satisfy_failed_allocation_helper(word_size, context, do_gc,
> >>>> clear_softrefs)
> >>>> attempt_allocation_at_safepoint(word_size, context, true);
> >>>> if (!do_gc)
> >>>> assert(!collector_policy()->should_clear_all_soft_refs());
> >>>> expand_and_allocate(word_size, context);
> >>>>
> >>>> This would make the code in satisfy_failed_allocation() a lot more
> >>>> clean.
> >>>>
> >>> In webrev.03 I decided to put same way as it was in the original code
> >>> (before any of my changes),
> >>> at the very end of method just when we already exhausted all
> >>> possibilities and about to return NULL.
> >>>
> >>> attempt_allocation_at_safepoint(word_size, context, false);
> >>> expand_and_allocate(word_size, context);
> >>> do_collection(false, false, word_size);
> >>> attempt_allocation_at_safepoint(word_size, context, true);
> >>> do_collection(false, true, word_size);
> >>> attempt_allocation_at_safepoint(word_size, context, true);
> >>> assert(!collector_policy()->should_clear_all_soft_refs());
> >>>
> >>> // What else? We might try synchronous finalization later. If the
> >>> total 1799 // space available is large enough for the allocation, then
> >>> a more
> >>> // complete compaction phase than we've tried so far might be
> >>> // appropriate.
> >>> assert(*succeeded, "sanity");
> >>> return NULL; 1804
> >>> }
> >>>
> >>> webrev.03 has this assert at same position, and I prefer it this way.
> >>>
[...]
> >> At least, in the helper, move the "assert(*succeeded, ...)" up.
> >>
> >> Like:
> >> --------------
> >>
> >> *succeeded = !GCLocker::check_active_before_gc(); // maybe use a temp
> >> variable here, that is not negated because we check the negated result
> >> again just later
> >>
> >> if (!*succeeded) {
> >> return NULL;
> >> }
> >>
> >> do_collection(....);
> >> assert(
> >> !collector_policy()->should_clear_all_soft_refs(), ...); // see above
> >>
> >> assert(*succeeded, "..."); // for the paranoid, but I would just skip
> >> it.
> >>
> >> result = attempt_allocation_at_safepoint(...);
> >>
> >> if (result != NULL) {
> >> return result;
> >> }
> >>
> >> result = expand_and_allocate(...);
> >>
> >> return result;
> >> ---------------
> >>
> >> (And the bool return value of do_collection removed).
> >>
> >> I also have a question about the order of
> >> attempt_allocation_at_safepoint() and the expand_and_allocate(), and
> >> could not find this answered before: so full gc might shrink the heap
> >> too much after full gc, and we should expand the heap.
> >> If I were to do this change, there would always (unconditionally) be an
> >> expansion after full gc, because the current change only expands if the
> >> allocation after gc fails. Also the 02 change.
> >>
> >> The review thread so far suggests that full gc might shrink too much,
> >> but this still seems to be the case here in some cases.
Could you clarify this question? To me, this looks like a potential bug
that has been overlooked so far in this change.
Thanks,
Thomas
More information about the hotspot-gc-dev
mailing list