RFR (S/M): 8155094: Add logging for long lasting methods found in JDK-8152948

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

RFR (S/M): 8155094: Add logging for long lasting methods found in JDK-8152948

Thomas Schatzl
Hi all,

  can I have reviews for this change that adds straightforward logging
for some phases that were found to sometimes take a significant amount
of time to allow us to identify them more easily in the future?

It's basic adding of the necessary code - I agree that we should make
the logging more convenient to use - but I would like to do this kind
of refactoring in another change.

CR:
https://bugs.openjdk.java.net/browse/JDK-8155094
Webrev:
http://cr.openjdk.java.net/~tschatzl/8155094/webrev/
Testing:
jprt (updated test case), tier 2+3 gc tests

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

Re: RFR (S/M): 8155094: Add logging for long lasting methods found in JDK-8152948

Kim Barrett
> On Feb 23, 2017, at 6:06 AM, Thomas Schatzl <[hidden email]> wrote:
>
> Hi all,
>
>   can I have reviews for this change that adds straightforward logging
> for some phases that were found to sometimes take a significant amount
> of time to allow us to identify them more easily in the future?
>
> It's basic adding of the necessary code - I agree that we should make
> the logging more convenient to use - but I would like to do this kind
> of refactoring in another change.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8155094
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8155094/webrev/
> Testing:
> jprt (updated test case), tier 2+3 gc tests
>
> Thanks,
>   Thomas

------------------------------------------------------------------------------
src/share/vm/gc/g1/g1GCPhaseTimes.hpp
 103   double _cur_dpt_update_time_ms;
 190   void record_dpt_update_time(double ms) {

What is dpt?  Oh, DerivedPointerTable.  The abbreviation is not
helpful; please expand.

------------------------------------------------------------------------------
src/share/vm/gc/g1/g1CollectedHeap.cpp
3004 void G1CollectedHeap::start_new_collection_set() {
...
3009   guarantee(_eden.length() == 0, "eden should have been cleared");
3010   g1_policy()->transfer_survivors_to_cset(survivor());

The call site for this new function at 3215 had those two lines, but
the call site at 1382 did not.  That change seem suspicious.

------------------------------------------------------------------------------

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S/M): 8155094: Add logging for long lasting methods found in JDK-8152948

Thomas Schatzl
Hi Kim,

  thanks for your review.

On Sun, 2017-02-26 at 03:03 -0500, Kim Barrett wrote:

> >
> > On Feb 23, 2017, at 6:06 AM, Thomas Schatzl <thomas.schatzl@oracle.
> > com> wrote:
> >
> > Hi all,
> >
> >   can I have reviews for this change that adds straightforward
> > logging for some phases that were found to sometimes take a
> > significant amount of time to allow us to identify them more easily
> > in the future?
> >
> > It's basic adding of the necessary code - I agree that we should
> > make the logging more convenient to use - but I would like to do
> > this kind of refactoring in another change.
> >
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8155094
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8155094/webrev/
> > Testing:
> > jprt (updated test case), tier 2+3 gc tests
> >
> > Thanks,
> >   Thomas
> -------------------------------------------------------------------
> ----------- 
> src/share/vm/gc/g1/g1GCPhaseTimes.hpp
>  103   double _cur_dpt_update_time_ms;
>  190   void record_dpt_update_time(double ms) {
>
> What is dpt?  Oh, DerivedPointerTable.  The abbreviation is not
> helpful; please expand.

Done.

>
> -------------------------------------------------------------------
> ----------- 
> src/share/vm/gc/g1/g1CollectedHeap.cpp
> 3004 void G1CollectedHeap::start_new_collection_set() {
> ...
> 3009   guarantee(_eden.length() == 0, "eden should have been
> cleared");
> 3010   g1_policy()->transfer_survivors_to_cset(survivor());
>
> The call site for this new function at 3215 had those two lines, but
> the call site at 1382 did not.  That change seem suspicious.
>

The two lines relabel the survivor regions as eden.
Since full gc always relabels everything else as old, so this is a
(cheap) nop at this time.

It may be useful though in the future to have full gc not mix together
the generations (i.e. prematurely promote the objects left in young gen
), resulting in separate survivor and old regions.

I can undo this change though.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S/M): 8155094: Add logging for long lasting methods found in JDK-8152948

Thomas Schatzl
Hi,

On Mon, 2017-03-06 at 14:46 +0100, Thomas Schatzl wrote:
> Hi Kim,

>   thanks for your review.
[...]


  I forgot to add the links to the new webrevs:

http://cr.openjdk.java.net/~tschatzl/8155094/webrev.0_to_1/ (incrementa
l)

http://cr.openjdk.java.net/~tschatzl/8155094/webrev.1/ (full)

Thanks,
  Thomas


Reply | Threaded
Open this post in threaded view
|

Re: RFR (S/M): 8155094: Add logging for long lasting methods found in JDK-8152948

Kim Barrett
> On Mar 6, 2017, at 8:52 AM, Thomas Schatzl <[hidden email]> wrote:
>
> Hi,
>
> On Mon, 2017-03-06 at 14:46 +0100, Thomas Schatzl wrote:
>>  Hi Kim,
>>  
>>    thanks for your review.
> [...]
>
>
>   I forgot to add the links to the new webrevs:
>
> http://cr.openjdk.java.net/~tschatzl/8155094/webrev.0_to_1/ (incrementa
> l)
>
> http://cr.openjdk.java.net/~tschatzl/8155094/webrev.1/ (full)
>
> Thanks,
>   Thomas

I take it you decided not to undo (some of) the start_new_collection_set() change?

Assuming that was intentional, looks good.

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S/M): 8155094: Add logging for long lasting methods found in JDK-8152948

Thomas Schatzl
Hi Kim,

On Tue, 2017-03-07 at 01:52 -0500, Kim Barrett wrote:

> >
> > On Mar 6, 2017, at 8:52 AM, Thomas Schatzl <[hidden email]
> > om> wrote:
> >
> > Hi,
> >
> > On Mon, 2017-03-06 at 14:46 +0100, Thomas Schatzl wrote:
> > >
> > >  Hi Kim,
> > >  
> > >    thanks for your review.
> > [...]
> >
> >
> >   I forgot to add the links to the new webrevs:
> >
> > http://cr.openjdk.java.net/~tschatzl/8155094/webrev.0_to_1/
> > (incrementa
> > l)
> >
> > http://cr.openjdk.java.net/~tschatzl/8155094/webrev.1/ (full)
> >
> > Thanks,
> >   Thomas
> I take it you decided not to undo (some of) the
> start_new_collection_set() change?
>
> Assuming that was intentional, looks good.
>

I kept these changes, that were indeed intentional. I was asking about
your opinion about this matter - it seemed appropriate to me do this
minor refactoring while in this code.

There is some additional time to think about this as I need a second
reviewer anyway.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S/M): 8155094: Add logging for long lasting methods found in JDK-8152948

sangheon.kim@oracle.com
Hi Thomas,

On 03/07/2017 12:24 AM, Thomas Schatzl wrote:

> Hi Kim,
>
> On Tue, 2017-03-07 at 01:52 -0500, Kim Barrett wrote:
>>> On Mar 6, 2017, at 8:52 AM, Thomas Schatzl <[hidden email]
>>> om> wrote:
>>>
>>> Hi,
>>>
>>> On Mon, 2017-03-06 at 14:46 +0100, Thomas Schatzl wrote:
>>>>   Hi Kim,
>>>>  
>>>>     thanks for your review.
>>> [...]
>>>
>>>
>>>    I forgot to add the links to the new webrevs:
>>>
>>> http://cr.openjdk.java.net/~tschatzl/8155094/webrev.0_to_1/
>>> (incrementa
>>> l)
>>>
>>> http://cr.openjdk.java.net/~tschatzl/8155094/webrev.1/ (full)
>>>
>>> Thanks,
>>>    Thomas
>> I take it you decided not to undo (some of) the
>> start_new_collection_set() change?
>>
>> Assuming that was intentional, looks good.
>>
> I kept these changes, that were indeed intentional. I was asking about
> your opinion about this matter - it seemed appropriate to me do this
> minor refactoring while in this code.
>
> There is some additional time to think about this as I need a second
> reviewer anyway.
webrev.1 (including minor refactoring) seems good to me.

Thanks,
Sangheon


>
> Thanks,
>    Thomas
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S/M): 8155094: Add logging for long lasting methods found in JDK-8152948

Thomas Schatzl
Hi Sangheon,

On Wed, 2017-03-15 at 15:37 -0700, sangheon wrote:

> Hi Thomas,
>
> On 03/07/2017 12:24 AM, Thomas Schatzl wrote:
> > On Tue, 2017-03-07 at 01:52 -0500, Kim Barrett wrote:
> > > > On Mar 6, 2017, at 8:52 AM, Thomas Schatzl
> > > > <[hidden email]
> > > > om> wrote:
> > > >
> > > > Hi,
> > > >
> > > > [...]
> > >
> > > Assuming that was intentional, looks good.
> > >
> > I kept these changes, that were indeed intentional. I was asking
> > about your opinion about this matter - it seemed appropriate to me
> > do this minor refactoring while in this code.
> >
> > There is some additional time to think about this as I need a
> > second reviewer anyway.
> webrev.1 (including minor refactoring) seems good to me.

  thanks for your review.

Thomas