Request for code review - 8130265: gctests/LargeObjects/large001 fails with OutOfMemoryError: Java heap space
Jesper Wilhelmsson
jesper.wilhelmsson at oracle.com
Tue Aug 25 20:15:46 UTC 2015
Den 25/8/15 kl. 20:36, skrev Alexander Harlap:
> I placed assert( assert(!collector_policy()->should_clear_all_soft_refs(), "Flag
> should have been handled and cleared prior to this point");
> according to Jesper's suggestion.
Ah, I see. I had a "if (!do_gc)" before the assert in my example so that it
would only be executed in the last of the three calls.
The latest change is good as is. If you want to move the assert into the helper
again it could be like:
if (do_gc) {
// Expansion didn't work, we'll try to do a Full GC.
*gc_succeeded = do_collection(false, /* explicit_gc */
clear_all_soft_refs,
word_size);
} else {
assert(!collector_policy()->should_clear_all_soft_refs(),
"Flag should have been handled and cleared prior to this point");
}
But looking at this code snippet, I think I like your latest version better.
Thanks,
/Jesper
>
> This assert was introduced by Jon in change 6b73e879f1c2.
>
> New version is here:
>
> http://cr.openjdk.java.net/~aharlap/8130265/webrev.05/
>
> Alex
>
>
>
> On 8/25/2015 9:25 AM, 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 enough to keep it there, at the end of satisfy_failed_allocation().
>>
>> 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.
>> /Jesper
>>
>>
>> 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.
>>>>>
>>>>>> * 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