Request for code review - 8130265: gctests/LargeObjects/large001 fails with OutOfMemoryError: Java heap space
Alexander Harlap
alexander.harlap at oracle.com
Mon Aug 24 21:24:14 UTC 2015
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.
>>
>>> * Unless I'm missing some subtle detail here, the usage of the
>>> variable 'succeeded' is unnecessary as 'result' will always contain
>>> the same information. I don't think we should pass on 'succeeded' to
>>> the helper, but simply set it afterwards before we return from
>>> satisfy_failed_allocation().
>> No, helper may return NULL in two differnt cases:
>> 1. Full GC failed, *succeeded = false, no attempts to allocate was
>> made
>> 2. Full GC succeeded, *succeeded = true, all attempts to allocate
>> failed
>>
>> So we need to pass 'succeeded' to the helper.
> Can we rename this to something more descriptive? I had to look into
> do_collection() what it means. Even the gc_succeeded name in the helper
> is better.
>
> Also, the only condition where do_collection() fails is if the gc locker
> is active. This return value is actually ignored in
> do_full_collection().
>
> So the gc locker check could be moved out of do_collection() (adding an
> assert there), and the value of gc_succeeded/succeeded determined using
> this check immediately.
>
> 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 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.
>
> Thanks,
> Thomas
>
>
More information about the hotspot-gc-dev
mailing list