Request for code review - 8130265: gctests/LargeObjects/large001 fails with OutOfMemoryError: Java heap space
Alexander Harlap
alexander.harlap at oracle.com
Tue Aug 25 18:36:19 UTC 2015
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.
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