[10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

[10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

Thomas Schatzl
Hi all,

  can I have reviews for this change that fixes a race within the G1
collector that in conjunction with the GC locker causes it to OoME
prematurely?

The issue is that if a young gc of any kind (including gc locker caused
ones) fails to allocate enough memory, the scheduled full gc can be
cancelled by the gc locker, causing OoME in that thread. I.e. before
any full gc has been tried.

The solution is to, similar to other collectors (which corresponds to
the observation that they do not show this behavior), start a
compacting full gc in the same VM operation where the young GC has been
 started in to avoid this race to happen.

Axel Siebenborn (cc'ed) from SAP originally contributed a fix that
covered most situations; this change enhances it (and converts the
standalone test he wrote into a jtreg ones in addition to style
changes). I added us both as contributors.

Unfortunately the test has had to be put into the Problemlist, because
I did not manage to make it not to exhibit the failure in JDK-8192647
sometimes. I still see it as a useful addition as soon that one has
been fixed. For testing I manually checked the test output that the GC
type "upgrade" occurs as expected though.

Also, the test has been changed to be a stress test, and only execute
at most a certain amount of time.

CR:
https://bugs.openjdk.java.net/browse/JDK-8137099
Webrev:
http://cr.openjdk.java.net/~tschatzl/8137099/webrev/
Testing:
hs-tier1-3, manual execution of included test, checking output to be as
expected.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

RE: [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

Siebenborn, Axel
Hi Thomas,
thanks for bringing this up again .

For me the change looks good.

Thanks,
Axel
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

Hohensee, Paul
In reply to this post by Thomas Schatzl
In attempt_allocation_humongous in g1CollectedHeap.cpp, there’s a note that says there’s code duplication between it and attempt_allocation_slow. There may be a case of version skew between the code at line 553 and the lack of similar code at line 972. If you don’t want to retry the humongous allocation attempt at that point, then the comment at line 968 should be updated.

Thanks,
Paul

On 12/4/17, 7:58 AM, "hotspot-gc-dev on behalf of Thomas Schatzl" <[hidden email] on behalf of [hidden email]> wrote:

    Hi all,
   
      can I have reviews for this change that fixes a race within the G1
    collector that in conjunction with the GC locker causes it to OoME
    prematurely?
   
    The issue is that if a young gc of any kind (including gc locker caused
    ones) fails to allocate enough memory, the scheduled full gc can be
    cancelled by the gc locker, causing OoME in that thread. I.e. before
    any full gc has been tried.
   
    The solution is to, similar to other collectors (which corresponds to
    the observation that they do not show this behavior), start a
    compacting full gc in the same VM operation where the young GC has been
     started in to avoid this race to happen.
   
    Axel Siebenborn (cc'ed) from SAP originally contributed a fix that
    covered most situations; this change enhances it (and converts the
    standalone test he wrote into a jtreg ones in addition to style
    changes). I added us both as contributors.
   
    Unfortunately the test has had to be put into the Problemlist, because
    I did not manage to make it not to exhibit the failure in JDK-8192647
    sometimes. I still see it as a useful addition as soon that one has
    been fixed. For testing I manually checked the test output that the GC
    type "upgrade" occurs as expected though.
   
    Also, the test has been changed to be a stress test, and only execute
    at most a certain amount of time.
   
    CR:
    https://bugs.openjdk.java.net/browse/JDK-8137099
    Webrev:
    http://cr.openjdk.java.net/~tschatzl/8137099/webrev/
    Testing:
    hs-tier1-3, manual execution of included test, checking output to be as
    expected.
   
    Thanks,
      Thomas
   
   

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

Thomas Schatzl
Hi Paul,

  sorry for the late reply, there has been another issue keeping me
busy ;)

On Thu, 2017-12-07 at 00:44 +0000, Hohensee, Paul wrote:
> In attempt_allocation_humongous in g1CollectedHeap.cpp, there’s a
> note that says there’s code duplication between it and
> attempt_allocation_slow. There may be a case of version skew between
> the code at line 553 and the lack of similar code at line 972. If you
> don’t want to retry the humongous allocation attempt at that point,
> then the comment at line 968 should be updated.

  humongous allocation needs to be under the heap lock anyway, so we
just skip the inline allocation as it would be the same as the code at
the start of the loop.

I did add a comment, fixed some whitespace and had to move declarations
in the JNI c file to make it compile on some platforms:

New webrevs:
http://cr.openjdk.java.net/~tschatzl/8137099/webrev.0_to_1 (diff)
http://cr.openjdk.java.net/~tschatzl/8137099/webrev.1 (full)

There is some potential to do refactoring in the
attempt_allocation_slow/humongous methods (the part in "if
(should_try_gc)"), but I would like to do that in a different change.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

Thomas Schatzl
In reply to this post by Siebenborn, Axel
Hi,

On Wed, 2017-12-06 at 15:23 +0000, Siebenborn, Axel wrote:
> Hi Thomas,
> thanks for bringing this up again .
>
> For me the change looks good.
>

  thanks for your review.

Thomas

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

Stefan Johansson
In reply to this post by Thomas Schatzl
Hi Thomas,

On 2017-12-11 12:07, Thomas Schatzl wrote:

> Hi Paul,
>
>    sorry for the late reply, there has been another issue keeping me
> busy ;)
>
> On Thu, 2017-12-07 at 00:44 +0000, Hohensee, Paul wrote:
>> In attempt_allocation_humongous in g1CollectedHeap.cpp, there’s a
>> note that says there’s code duplication between it and
>> attempt_allocation_slow. There may be a case of version skew between
>> the code at line 553 and the lack of similar code at line 972. If you
>> don’t want to retry the humongous allocation attempt at that point,
>> then the comment at line 968 should be updated.
>    humongous allocation needs to be under the heap lock anyway, so we
> just skip the inline allocation as it would be the same as the code at
> the start of the loop.
>
> I did add a comment, fixed some whitespace and had to move declarations
> in the JNI c file to make it compile on some platforms:
>
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8137099/webrev.0_to_1 (diff)
> http://cr.openjdk.java.net/~tschatzl/8137099/webrev.1 (full)
Thanks for taking on this problem. I think the change looks really good,
just a few comments:
---
g1CollectedHeap.cpp
* The logging uses the thread pointer as identifier, I think it would be
more useful to use the thread name. Also this one log line is only
tagged with "gc" while the others use "gc,alloc":
527       log_trace(gc)(PTR_FORMAT " Unsuccessfully scheduled collection
allocating " SIZE_FORMAT " words",
528                     p2i(Thread::current()), word_size);
---
g1CollectedHeap.hpp
* Remove "friend class VM_G1CollectForAllocation;"
* Remove/change:
   // Callback from VM_G1CollectForAllocation operation.
   // This function does everything necessary/possible to satisfy a
   // failed allocation request (including collection, expansion, etc.)
   HeapWord* satisfy_failed_allocation(size_t word_size,
                                       AllocationContext_t context,
                                       bool* succeeded);
---
vm_operations_g1.hpp
* Remove "//     - VM_G1CollectForAllocation"

If you like you could also simplify the class hierarchy now, since
VM_G1OperationWithAllocRequest only has one sub-class.
---

Thanks,
Stefan

> There is some potential to do refactoring in the
> attempt_allocation_slow/humongous methods (the part in "if
> (should_try_gc)"), but I would like to do that in a different change.
>
> Thanks,
>    Thomas
>

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

Thomas Schatzl
Hi Stefan,

  thanks for your review.

On Mon, 2017-12-11 at 14:19 +0100, Stefan Johansson wrote:

> Hi Thomas,
>
> On 2017-12-11 12:07, Thomas Schatzl wrote:
> > Hi Paul,
> >
> >    sorry for the late reply, there has been another issue keeping
> > me
> > busy ;)
> >
> > On Thu, 2017-12-07 at 00:44 +0000, Hohensee, Paul wrote:
> > > In attempt_allocation_humongous in g1CollectedHeap.cpp, there’s a
> > > note that says there’s code duplication between it and
> > > attempt_allocation_slow. There may be a case of version skew
> > > between the code at line 553 and the lack of similar code at line
> > > 972. If you don’t want to retry the humongous allocation attempt
> > > at that point, then the comment at line 968 should be updated.
> >
> >    humongous allocation needs to be under the heap lock anyway, so
> > we just skip the inline allocation as it would be the same as the
> > code at the start of the loop.
> >
> > I did add a comment, fixed some whitespace and had to move
> > declarations in the JNI c file to make it compile on some
> > platforms:
> >
> > New webrevs:
> > http://cr.openjdk.java.net/~tschatzl/8137099/webrev.0_to_1 (diff)
> > http://cr.openjdk.java.net/~tschatzl/8137099/webrev.1 (full)
>
> Thanks for taking on this problem. I think the change looks really
> good, just a few comments:
> ---
> g1CollectedHeap.cpp
> * The logging uses the thread pointer as identifier, I think it would
> be more useful to use the thread name. Also this one log line is
> only tagged with "gc" while the others use "gc,alloc":
> 527       log_trace(gc)(PTR_FORMAT " Unsuccessfully scheduled
> collection allocating " SIZE_FORMAT " words",
> 528                     p2i(Thread::current()), word_size);

done

> ---
> g1CollectedHeap.hpp
> * Remove "friend class VM_G1CollectForAllocation;"

Done

> * Remove/change:
>    // Callback from VM_G1CollectForAllocation operation.
>    // This function does everything necessary/possible to satisfy a
>    // failed allocation request (including collection, expansion,
> etc.)
>    HeapWord* satisfy_failed_allocation(size_t word_size,
>                                        AllocationContext_t context,
>                                        bool* succeeded);

Fine now :)

> ---
> vm_operations_g1.hpp
> * Remove "//     - VM_G1CollectForAllocation"
>
> If you like you could also simplify the class hierarchy now, since
> VM_G1OperationWithAllocRequest only has one sub-class.

Done.

- I changed the name of VM_G1IncCollectionPause to
VM_G1CollectForAllocation (the name of the operation that has been
deleted previously) because it is more fitting

- un-scrambled the parameters to the VM_G1CollectForAllocation
constructor

Testing with hs-tier1-5

New webrevs:
http://cr.openjdk.java.net/~tschatzl/8137099/webrev.1_to_2/ (diff)
http://cr.openjdk.java.net/~tschatzl/8137099/webrev.2/ (full)

Thanks,
  Thomas
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

Thomas Schatzl
Hi all,

  Erik Duveblad had some offline comments:

New webrevs:
http://cr.openjdk.java.net/~tschatzl/8137099/webrev.2_to_3/ (diff)
http://cr.openjdk.java.net/~tschatzl/8137099/webrev.3/ (full)

- inverting some conditions in the clauses to read better

- extract out the condition to do the maximally compacting full gc
added in this change into a separate method.

Erik Duveblad also noted that this change contains some slight
behavioral change in when a collection is started. I.e. previously TLAB
allocation by itself could not cause a GC. Since this change is already
quite big, he suggested to fix this in a follow-up, and push them
together.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

Thomas Schatzl
Hi again,

On Tue, 2018-01-09 at 10:52 +0100, Thomas Schatzl wrote:

> Hi all,
>
>   Erik Duveblad had some offline comments:
>
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8137099/webrev.2_to_3/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8137099/webrev.3/ (full)
>
> - inverting some conditions in the clauses to read better
>
> - extract out the condition to do the maximally compacting full gc
> added in this change into a separate method.
>
> Erik Duveblad also noted that this change contains some slight
> behavioral change in when a collection is started. I.e. previously
> TLAB allocation by itself could not cause a GC. Since this change is
> already quite big, he suggested to fix this in a follow-up, and push
> them together.

  additionally we agreed to retarget both changes to 11 to let it bake
a bit.

Thanks,
  Thomas
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

Thomas Schatzl
Hi again,

 :(

On Tue, 2018-01-09 at 10:54 +0100, Thomas Schatzl wrote:

> Hi again,
>
> On Tue, 2018-01-09 at 10:52 +0100, Thomas Schatzl wrote:
> > Hi all,
> >
> >   Erik Duveblad had some offline comments:
> >
> > New webrevs:
> > http://cr.openjdk.java.net/~tschatzl/8137099/webrev.2_to_3/ (diff)
> > http://cr.openjdk.java.net/~tschatzl/8137099/webrev.3/ (full)
> >
> > - inverting some conditions in the clauses to read better
> >
> > - extract out the condition to do the maximally compacting full gc
> > added in this change into a separate method.
> >
> > Erik Duveblad also noted that this change contains some slight
> > behavioral change in when a collection is started. I.e. previously
> > TLAB allocation by itself could not cause a GC. Since this change
> > is already quite big, he suggested to fix this in a follow-up, and
> > push them together.

Some detail got lost here: previously TLAB allocation could not result
in a Full GC, it can now (that maximally compacting GC).

Its side effects need to be investigated a bit more.

>
>   additionally we agreed to retarget both changes to 11 to let it
> bake a bit.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

Thomas Schatzl
Hi,

  aaaand,

On Tue, 2018-01-09 at 11:11 +0100, Thomas Schatzl wrote:
> Hi again,
>
[...]

> > > Erik Duveblad also noted that this change contains some slight
> > > behavioral change in when a collection is started. I.e.
> > > previously TLAB allocation by itself could not cause a GC. Since
> > > this change is already quite big, he suggested to fix this in a
> > > follow-up, and push them together.
>
> Some detail got lost here: previously TLAB allocation could not
> result in a Full GC, it can now (that maximally compacting GC).
>
> Its side effects need to be investigated a bit more.

The only side effect would be that these collections would not be
accounted in the GC overhead limit mechanism (go OOME when doing too
many GCs in a row).

However, G1 never implemented the GC overhead limit mechanism. So this
behavioral change is a non-issue because it has no visible impact. I
filed RFE JDK-8194821 for that.

Serial and CMS have this issue: they do not account GCs caused by TLAB
allocation at all in the GC overhead limit. Filed JDK-8194823.

Only Parallel GC seems to do the right thing by not doing any GC during
TLAB allocation.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

Stefan Johansson
In reply to this post by Thomas Schatzl


On 2018-01-09 10:54, Thomas Schatzl wrote:

> Hi again,
>
> On Tue, 2018-01-09 at 10:52 +0100, Thomas Schatzl wrote:
>> Hi all,
>>
>>    Erik Duveblad had some offline comments:
>>
>> New webrevs:
>> http://cr.openjdk.java.net/~tschatzl/8137099/webrev.2_to_3/ (diff)
>> http://cr.openjdk.java.net/~tschatzl/8137099/webrev.3/ (full)
>>
>> - inverting some conditions in the clauses to read better
>>
>> - extract out the condition to do the maximally compacting full gc
>> added in this change into a separate method.
>>
>> Erik Duveblad also noted that this change contains some slight
>> behavioral change in when a collection is started. I.e. previously
>> TLAB allocation by itself could not cause a GC. Since this change is
>> already quite big, he suggested to fix this in a follow-up, and push
>> them together.
>    additionally we agreed to retarget both changes to 11 to let it bake
> a bit.
The latest changes looks good and I agree that doing this early in 11 is
good.

Thanks,
Stefan
>
> Thanks,
>    Thomas

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

Erik Helin-2
In reply to this post by Thomas Schatzl
Hi Thomas,

thanks for taking on this work and cleaning this up!

On 01/09/2018 10:52 AM, Thomas Schatzl wrote:
> Hi all,
>
>    Erik Duveblad had some offline comments:
>
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8137099/webrev.2_to_3/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8137099/webrev.3/ (full)

this looks good to me now, pending one small comment: please rename
G1CollectedHeap::no_more_regions_left_for_allocation to
G1CollectedHeap::has_regions_left_for_allocation (and of course change
the method). This way you can use the ! operator in the if condition,
which reads a bit easier.

And thanks for putting this into 11, it is the right decision IMO.

Thanks,
Erik

> - inverting some conditions in the clauses to read better
>
> - extract out the condition to do the maximally compacting full gc
> added in this change into a separate method.
>
> Erik Duveblad also noted that this change contains some slight
> behavioral change in when a collection is started. I.e. previously TLAB
> allocation by itself could not cause a GC. Since this change is already
> quite big, he suggested to fix this in a follow-up, and push them
> together.
>
> Thanks,
>    Thomas
>
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

Thomas Schatzl
Hi Erik,

On Wed, 2018-01-10 at 16:50 +0100, Erik Helin wrote:
> Hi Thomas,
>
> thanks for taking on this work and cleaning this up!

thanks for your review.

>
> On 01/09/2018 10:52 AM, Thomas Schatzl wrote:
> > Hi all,
> >
> >    Erik Duveblad had some offline comments:
> >
> > New webrevs:
> > http://cr.openjdk.java.net/~tschatzl/8137099/webrev.2_to_3/ (diff)
> > http://cr.openjdk.java.net/~tschatzl/8137099/webrev.3/ (full)
>
> this looks good to me now, pending one small comment: please rename
> G1CollectedHeap::no_more_regions_left_for_allocation to
> G1CollectedHeap::has_regions_left_for_allocation (and of course
> change the method). This way you can use the ! operator in the if
> condition, which reads a bit easier.
>

New webrev:
http://cr.openjdk.java.net/~tschatzl/8137099/webrev.3_to_4/ (diff)
http://cr.openjdk.java.net/~tschatzl/8137099/webrev.4/ (full)

> And thanks for putting this into 11, it is the right decision IMO.
>

No problem.

Thanks,
  Thomas

> Thanks,
> Erik
>
> > - inverting some conditions in the clauses to read better
> >
> > - extract out the condition to do the maximally compacting full gc
> > added in this change into a separate method.
> >
> > Erik Duveblad also noted that this change contains some slight
> > behavioral change in when a collection is started. I.e. previously
> > TLAB
> > allocation by itself could not cause a GC. Since this change is
> > already
> > quite big, he suggested to fix this in a follow-up, and push them
> > together.
> >
> > Thanks,
> >    Thomas
> >

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

Erik Helin-2
On 01/10/2018 05:47 PM, Thomas Schatzl wrote:

> Hi Erik,
>
> On Wed, 2018-01-10 at 16:50 +0100, Erik Helin wrote:
>> Hi Thomas,
>>
>> thanks for taking on this work and cleaning this up!
>
> thanks for your review.
>
>>
>> On 01/09/2018 10:52 AM, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>     Erik Duveblad had some offline comments:
>>>
>>> New webrevs:
>>> http://cr.openjdk.java.net/~tschatzl/8137099/webrev.2_to_3/ (diff)
>>> http://cr.openjdk.java.net/~tschatzl/8137099/webrev.3/ (full)
>>
>> this looks good to me now, pending one small comment: please rename
>> G1CollectedHeap::no_more_regions_left_for_allocation to
>> G1CollectedHeap::has_regions_left_for_allocation (and of course
>> change the method). This way you can use the ! operator in the if
>> condition, which reads a bit easier.
>>
>
> New webrev:
> http://cr.openjdk.java.net/~tschatzl/8137099/webrev.3_to_4/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8137099/webrev.4/ (full)

Looks good, Reviewed.

Thanks!
Erik

>> And thanks for putting this into 11, it is the right decision IMO.
>>
>
> No problem.
>
> Thanks,
>    Thomas
>
>> Thanks,
>> Erik
>>
>>> - inverting some conditions in the clauses to read better
>>>
>>> - extract out the condition to do the maximally compacting full gc
>>> added in this change into a separate method.
>>>
>>> Erik Duveblad also noted that this change contains some slight
>>> behavioral change in when a collection is started. I.e. previously
>>> TLAB
>>> allocation by itself could not cause a GC. Since this change is
>>> already
>>> quite big, he suggested to fix this in a follow-up, and push them
>>> together.
>>>
>>> Thanks,
>>>     Thomas
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

Stefan Johansson


On 2018-01-10 20:15, Erik Helin wrote:

> On 01/10/2018 05:47 PM, Thomas Schatzl wrote:
>> Hi Erik,
>>
>> On Wed, 2018-01-10 at 16:50 +0100, Erik Helin wrote:
>>> Hi Thomas,
>>>
>>> thanks for taking on this work and cleaning this up!
>>
>> thanks for your review.
>>
>>>
>>> On 01/09/2018 10:52 AM, Thomas Schatzl wrote:
>>>> Hi all,
>>>>
>>>>     Erik Duveblad had some offline comments:
>>>>
>>>> New webrevs:
>>>> http://cr.openjdk.java.net/~tschatzl/8137099/webrev.2_to_3/ (diff)
>>>> http://cr.openjdk.java.net/~tschatzl/8137099/webrev.3/ (full)
>>>
>>> this looks good to me now, pending one small comment: please rename
>>> G1CollectedHeap::no_more_regions_left_for_allocation to
>>> G1CollectedHeap::has_regions_left_for_allocation (and of course
>>> change the method). This way you can use the ! operator in the if
>>> condition, which reads a bit easier.
>>>
>>
>> New webrev:
>> http://cr.openjdk.java.net/~tschatzl/8137099/webrev.3_to_4/ (diff)
>> http://cr.openjdk.java.net/~tschatzl/8137099/webrev.4/ (full)
>
> Looks good, Reviewed.
>
+1
Stefan

> Thanks!
> Erik
>
>>> And thanks for putting this into 11, it is the right decision IMO.
>>>
>>
>> No problem.
>>
>> Thanks,
>>    Thomas
>>
>>> Thanks,
>>> Erik
>>>
>>>> - inverting some conditions in the clauses to read better
>>>>
>>>> - extract out the condition to do the maximally compacting full gc
>>>> added in this change into a separate method.
>>>>
>>>> Erik Duveblad also noted that this change contains some slight
>>>> behavioral change in when a collection is started. I.e. previously
>>>> TLAB
>>>> allocation by itself could not cause a GC. Since this change is
>>>> already
>>>> quite big, he suggested to fix this in a follow-up, and push them
>>>> together.
>>>>
>>>> Thanks,
>>>>     Thomas
>>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR (M/L): 8137099: G1 needs to "upgrade" GC within the safepoint if it can't allocate during that safepoint to avoid OoME

Thomas Schatzl
Hi everybody,

On Thu, 2018-01-11 at 09:37 +0100, Stefan Johansson wrote:

>
> On 2018-01-10 20:15, Erik Helin wrote:
> > On 01/10/2018 05:47 PM, Thomas Schatzl wrote:
> > > Hi Erik,
> > >
> > > On Wed, 2018-01-10 at 16:50 +0100, Erik Helin wrote:
> > > > Hi Thomas,
> > > >
> > > > thanks for taking on this work and cleaning this up!
> > >
> > > thanks for your review.
[...]
> > Looks good, Reviewed.
> >
>
> +1
> Stefan
> > Thanks!
> > Erik


Thanks for your reviews.

Thomas