Request for code review - 8130265: gctests/LargeObjects/large001 fails with OutOfMemoryError: Java heap space
Alexander Harlap
alexander.harlap at oracle.com
Thu Aug 27 00:00:29 UTC 2015
On 8/26/2015 6:26 AM, Thomas Schatzl wrote:
> 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.
Yes, I would agree to make a separate CR. Only I will need help to
specify issue.
>
>> 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.
I am sorry, I can not locate those defects - did you mean lines 1777 and
1804 in webrev.05?
Alex
>> 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.
Yes, full GC in some situation may over-shrink heap such way that heap
expansion will be needed. It was reason for
gctests/LargeObjects/large001 failure. Still I prefer to do expansion
only after allocation attempt failed. Why should we do unconditional
expansion ( even if it is not needed )? In the existing code we have
call to expand_and_allocate() only after
attempt_allocation_at_safepoint() failure. What potential bug do you mean?
> Thanks,
> Thomas
>
More information about the hotspot-gc-dev
mailing list