RFR (S): 8022872: G1: Use correct GC cause for young GC triggered by humongous allocations
Hi All, Could I have a couple for reviews for this small change? http://cr.openjdk.java.net/~brutisso/8022872/webrev.00/ This change makes sure that we report "humongous allocation" as the GC cause when we trigger a young collection because we are attempting a humongous allocation. Before the change a log entry for such a young GC looked like this: [GC pause (G1 Evacuation Pause) (young) 1364M->1364M(2048M), 0.0004200 secs] After the change it looks like this: [GC pause (G1 Humongous Allocation) (young) 1364M->1364M(2047M), 0.0541270 secs] While doing this change I realized that the GC cause _g1_inc_collection_pause was only used in one more place. The slow path for allocations. There is already a cause called _allocation_failure, so I suggest that we use that one instead. This will eliminate the need for _g1_inc_collection_pause. In that case the webrev will look like this: http://cr.openjdk.java.net/~brutisso/8022872/webrev_incCause/ I prefer this cleanup, but I'm fine with just doing the first one if we think _g1_inc_collection_pause cause to keep for some reason. Thanks, Bengt
Hi Bengt, (surprise!) This is a useful change. Both webrevs look good. I have a slight preference to the first one. I didn't like the use of _allocation_failure on the second one here: 984 result = do_collection_pause(word_size, gc_count_before, &succeeded, 985 GCCause::_allocation_failure); given that the allocation didn't really fail - the eden is just full. The other use of _allocation_failure (in the VM_G1CollectorForAllocation closure) really implies that an allocation did fail and G1 is trying to take evasive action. Tony On 8/21/13 7:47 AM, Bengt Rutisson wrote:
Hi All,
Could I have a couple for reviews for this small change?
http://cr.openjdk.java.net/~brutisso/8022872/webrev.00/
This change makes sure that we report "humongous allocation" as the GC cause when we trigger a young collection because we are attempting a humongous allocation.
Before the change a log entry for such a young GC looked like this:
[GC pause (G1 Evacuation Pause) (young) 1364M->1364M(2048M), 0.0004200 secs]
After the change it looks like this:
[GC pause (G1 Humongous Allocation) (young) 1364M->1364M(2047M), 0.0541270 secs]
While doing this change I realized that the GC cause _g1_inc_collection_pause was only used in one more place. The slow path for allocations. There is already a cause called _allocation_failure, so I suggest that we use that one instead. This will eliminate the need for _g1_inc_collection_pause.
In that case the webrev will look like this:
http://cr.openjdk.java.net/~brutisso/8022872/webrev_incCause/
I prefer this cleanup, but I'm fine with just doing the first one if we think _g1_inc_collection_pause cause to keep for some reason.
Thanks, Bengt
-- Tony Printezis | Staff Software Engineer | Twitter @TonyPrintezis tprintezis@twitter.com
Hi Tony, On 8/21/13 6:30 PM, Tony Printezis wrote:
Hi Bengt,
(surprise!)
A nice surprise! But a surprise indeed :)
This is a useful change. Both webrevs look good. I have a slight preference to the first one. I didn't like the use of _allocation_failure on the second one here:
984 result = do_collection_pause(word_size, gc_count_before, &succeeded, 985 GCCause::_allocation_failure);
given that the allocation didn't really fail - the eden is just full. The other use of _allocation_failure (in the VM_G1CollectorForAllocation closure) really implies that an allocation did fail and G1 is trying to take evasive action.
Thanks for looking at this! This was exactly the kind of feedback I was looking for. As I already mentioned I'm also fine with the first webrev and with this explanation I'm also in favor of that one. Unless anyone else has opinions I'll go with that one. Just need one more review... Bengt
Tony
On 8/21/13 7:47 AM, Bengt Rutisson wrote:
Hi All,
Could I have a couple for reviews for this small change?
http://cr.openjdk.java.net/~brutisso/8022872/webrev.00/
This change makes sure that we report "humongous allocation" as the GC cause when we trigger a young collection because we are attempting a humongous allocation.
Before the change a log entry for such a young GC looked like this:
[GC pause (G1 Evacuation Pause) (young) 1364M->1364M(2048M), 0.0004200 secs]
After the change it looks like this:
[GC pause (G1 Humongous Allocation) (young) 1364M->1364M(2047M), 0.0541270 secs]
While doing this change I realized that the GC cause _g1_inc_collection_pause was only used in one more place. The slow path for allocations. There is already a cause called _allocation_failure, so I suggest that we use that one instead. This will eliminate the need for _g1_inc_collection_pause.
In that case the webrev will look like this:
http://cr.openjdk.java.net/~brutisso/8022872/webrev_incCause/
I prefer this cleanup, but I'm fine with just doing the first one if we think _g1_inc_collection_pause cause to keep for some reason.
Thanks, Bengt
Hi, On Wed, 2013-08-21 at 21:21 +0200, Bengt Rutisson wrote:
Hi Tony,
On 8/21/13 6:30 PM, Tony Printezis wrote:
Hi Bengt,
(surprise!)
A nice surprise! But a surprise indeed :)
This is a useful change. Both webrevs look good. I have a slight preference to the first one. I didn't like the use of _allocation_failure on the second one here:
984 result = do_collection_pause(word_size, gc_count_before, &succeeded, 985 GCCause::_allocation_failure);
given that the allocation didn't really fail - the eden is just full. The other use of _allocation_failure (in the VM_G1CollectorForAllocation closure) really implies that an allocation did fail and G1 is trying to take evasive action.
Thanks for looking at this! This was exactly the kind of feedback I was looking for. As I already mentioned I'm also fine with the first webrev and with this explanation I'm also in favor of that one. Unless anyone else has opinions I'll go with that one. Just need one more review...
Here it is. I agree to keep the _g1_inc_collection_pause gc cause. Change looks good. Hth, Thomas
Thanks for the reviews Tony and Thomas! Bengt On 8/21/13 9:49 PM, Thomas Schatzl wrote:
Hi,
On Wed, 2013-08-21 at 21:21 +0200, Bengt Rutisson wrote:
Hi Tony,
On 8/21/13 6:30 PM, Tony Printezis wrote:
Hi Bengt,
(surprise!) A nice surprise! But a surprise indeed :)
This is a useful change. Both webrevs look good. I have a slight preference to the first one. I didn't like the use of _allocation_failure on the second one here:
984 result = do_collection_pause(word_size, gc_count_before, &succeeded, 985 GCCause::_allocation_failure);
given that the allocation didn't really fail - the eden is just full. The other use of _allocation_failure (in the VM_G1CollectorForAllocation closure) really implies that an allocation did fail and G1 is trying to take evasive action. Thanks for looking at this! This was exactly the kind of feedback I was looking for. As I already mentioned I'm also fine with the first webrev and with this explanation I'm also in favor of that one. Unless anyone else has opinions I'll go with that one. Just need one more review... Here it is. I agree to keep the _g1_inc_collection_pause gc cause.
Change looks good.
Hth, Thomas
Glad it was a nice surprise. :-) BTW, does anyone (Mark?) know whether I can re-instate my OpenJDK membership and/or reviewer status? Or do I start from scratch? Thanks, Tony On 8/21/13 12:21 PM, Bengt Rutisson wrote:
Hi Tony,
On 8/21/13 6:30 PM, Tony Printezis wrote:
Hi Bengt,
(surprise!)
A nice surprise! But a surprise indeed :)
This is a useful change. Both webrevs look good. I have a slight preference to the first one. I didn't like the use of _allocation_failure on the second one here:
984 result = do_collection_pause(word_size, gc_count_before, &succeeded, 985 GCCause::_allocation_failure);
given that the allocation didn't really fail - the eden is just full. The other use of _allocation_failure (in the VM_G1CollectorForAllocation closure) really implies that an allocation did fail and G1 is trying to take evasive action.
Thanks for looking at this! This was exactly the kind of feedback I was looking for. As I already mentioned I'm also fine with the first webrev and with this explanation I'm also in favor of that one. Unless anyone else has opinions I'll go with that one. Just need one more review...
Bengt
Tony
On 8/21/13 7:47 AM, Bengt Rutisson wrote:
Hi All,
Could I have a couple for reviews for this small change?
http://cr.openjdk.java.net/~brutisso/8022872/webrev.00/
This change makes sure that we report "humongous allocation" as the GC cause when we trigger a young collection because we are attempting a humongous allocation.
Before the change a log entry for such a young GC looked like this:
[GC pause (G1 Evacuation Pause) (young) 1364M->1364M(2048M), 0.0004200 secs]
After the change it looks like this:
[GC pause (G1 Humongous Allocation) (young) 1364M->1364M(2047M), 0.0541270 secs]
While doing this change I realized that the GC cause _g1_inc_collection_pause was only used in one more place. The slow path for allocations. There is already a cause called _allocation_failure, so I suggest that we use that one instead. This will eliminate the need for _g1_inc_collection_pause.
In that case the webrev will look like this:
http://cr.openjdk.java.net/~brutisso/8022872/webrev_incCause/
I prefer this cleanup, but I'm fine with just doing the first one if we think _g1_inc_collection_pause cause to keep for some reason.
Thanks, Bengt
-- Tony Printezis | Staff Software Engineer | Twitter @TonyPrintezis tprintezis@twitter.com
According to this page you are still a Reviewer for the HSX project and the JDK projects: http://openjdk.java.net/census#tonyp Bengt On 8/21/13 10:32 PM, Tony Printezis wrote:
Glad it was a nice surprise. :-)
BTW, does anyone (Mark?) know whether I can re-instate my OpenJDK membership and/or reviewer status? Or do I start from scratch?
Thanks,
Tony
On 8/21/13 12:21 PM, Bengt Rutisson wrote:
Hi Tony,
On 8/21/13 6:30 PM, Tony Printezis wrote:
Hi Bengt,
(surprise!)
A nice surprise! But a surprise indeed :)
This is a useful change. Both webrevs look good. I have a slight preference to the first one. I didn't like the use of _allocation_failure on the second one here:
984 result = do_collection_pause(word_size, gc_count_before, &succeeded, 985 GCCause::_allocation_failure);
given that the allocation didn't really fail - the eden is just full. The other use of _allocation_failure (in the VM_G1CollectorForAllocation closure) really implies that an allocation did fail and G1 is trying to take evasive action.
Thanks for looking at this! This was exactly the kind of feedback I was looking for. As I already mentioned I'm also fine with the first webrev and with this explanation I'm also in favor of that one. Unless anyone else has opinions I'll go with that one. Just need one more review...
Bengt
Tony
On 8/21/13 7:47 AM, Bengt Rutisson wrote:
Hi All,
Could I have a couple for reviews for this small change?
http://cr.openjdk.java.net/~brutisso/8022872/webrev.00/
This change makes sure that we report "humongous allocation" as the GC cause when we trigger a young collection because we are attempting a humongous allocation.
Before the change a log entry for such a young GC looked like this:
[GC pause (G1 Evacuation Pause) (young) 1364M->1364M(2048M), 0.0004200 secs]
After the change it looks like this:
[GC pause (G1 Humongous Allocation) (young) 1364M->1364M(2047M), 0.0541270 secs]
While doing this change I realized that the GC cause _g1_inc_collection_pause was only used in one more place. The slow path for allocations. There is already a cause called _allocation_failure, so I suggest that we use that one instead. This will eliminate the need for _g1_inc_collection_pause.
In that case the webrev will look like this:
http://cr.openjdk.java.net/~brutisso/8022872/webrev_incCause/
I prefer this cleanup, but I'm fine with just doing the first one if we think _g1_inc_collection_pause cause to keep for some reason.
Thanks, Bengt
2013/8/21 6:32 -0700, tprintezis@twitter.com:
BTW, does anyone (Mark?) know whether I can re-instate my OpenJDK membership and/or reviewer status? Or do I start from scratch?
Your OpenJDK roles stay with you, not with your employer, so you still have all the roles you had on the day you left Oracle. You don't need to submit an OCA since Twitter has signed the OCA for all its employees. If you want to commit code, send a new SSH key to keys@openjdk.java.net. - Mark
Mark and Bengt, Thanks for the replies. I think I confused "roles" and "members" when I read the OpenJDK bylaws. Tony On 8/21/13 2:01 PM, mark.reinhold@oracle.com wrote:
2013/8/21 6:32 -0700, tprintezis@twitter.com:
BTW, does anyone (Mark?) know whether I can re-instate my OpenJDK membership and/or reviewer status? Or do I start from scratch? Your OpenJDK roles stay with you, not with your employer, so you still have all the roles you had on the day you left Oracle.
You don't need to submit an OCA since Twitter has signed the OCA for all its employees.
If you want to commit code, send a new SSH key to keys@openjdk.java.net.
- Mark
-- Tony Printezis | Staff Software Engineer | Twitter @TonyPrintezis tprintezis@twitter.com
2013/8/21 7:57 -0700, tprintezis@twitter.com:
Thanks for the replies. I think I confused "roles" and "members" when I read the OpenJDK bylaws.
Hmm. Well, just to be clear, here's your entry in the census: http://openjdk.java.net/census#tonyp I'll update your entry in the contributor database to use your shiny new Twitter e-mail address. - Mark
participants (4)
-
Bengt Rutisson
-
mark.reinhold＠oracle.com
-
Thomas Schatzl
-
Tony Printezis