RFR (M): 8190426: Lazily initialize refinement threads with UseDynamicNumberOfGCThreads

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

RFR (M): 8190426: Lazily initialize refinement threads with UseDynamicNumberOfGCThreads

Thomas Schatzl
Hi,

  can I have reviews for this change that makes refinement threads
behave the same as the other thread groups we have in G1?

I.e. with UseDynamicNumberOfGCThreads enabled (which is off by default)
they are created lazily.

This helps a little bit further with startup/footprint benchmarks (if
enabled).

CR:
https://bugs.openjdk.java.net/browse/JDK-8190426
Webrev:
http://cr.openjdk.java.net/~tschatzl/8190426/webrev/
Testing:
hs-tier1+2

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

Re: RFR (M): 8190426: Lazily initialize refinement threads with UseDynamicNumberOfGCThreads

sangheon.kim@oracle.com
Hi Thomas,

On 11/03/2017 09:28 AM, Thomas Schatzl wrote:

> Hi,
>
>    can I have reviews for this change that makes refinement threads
> behave the same as the other thread groups we have in G1?
>
> I.e. with UseDynamicNumberOfGCThreads enabled (which is off by default)
> they are created lazily.
>
> This helps a little bit further with startup/footprint benchmarks (if
> enabled).
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8190426
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8190426/webrev/
> Testing:
> hs-tier1+2
I like this idea, thank you for improving this.

I have some comments.
------------------------------------------------
src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp
37   _cg1r(NULL),
- Probably _cr or _g1cr.

54   assert(cg1r != NULL, "Passed g1ConcurrentRefine must not be NULL");
- G1ConcurrentRefine not g1ConcurrentRefine.

57   _threads = NEW_C_HEAP_ARRAY(G1ConcurrentRefineThread*,
num_max_threads, mtGC);
58   for (uint i = 0; i < num_max_threads; i++) {
- Don't we need NULL check at line 58?

67 void G1ConcurrentRefineThreadControl::maybe_activate_next(uint
cur_worker_id) {
- without 'next' seems better or 'maybe_activate_thread()'.

68   assert(cur_worker_id < _num_max_threads, "Tried to activate from
impossible thread %u", cur_worker_
- I would prefer more detailed logging rather than 'impossible'. e.g.
Activating current worker id exceeds maximum number of threads.

78     _threads[worker_id] = new G1ConcurrentRefineThread(_cg1r, worker_id);
79     thread_to_activate = _threads[worker_id];
- NULL check at line 79?
- If we fail to allocate and G1ConcRefinementThreads is set, printing
warning message would needed.

81   thread_to_activate->activate();
- Probably NULL check is needed for 'thread_to_activate'.

198   _thread_control.initialize(this, max_num_threads());
- G1ConcurrentRefine::create() seems proper location to treat allocation
failure.

389 void G1ConcurrentRefine::maybe_activate_more_threads(uint worker_id,
size_t num_cur_buffers) {
- maybe_activate_more_thread() as this will activate only 1 thread.

------------------------------------------------
src/hotspot/share/gc/g1/g1ConcurrentRefineThread.hpp
62   G1ConcurrentRefineThread(G1ConcurrentRefine* cg1r, uint worker_id);
- cg1r -> cr or g1cr?

Thanks,
Sangheon


> Thanks,
>    Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR (M): 8190426: Lazily initialize refinement threads with UseDynamicNumberOfGCThreads

Thomas Schatzl
Hi Sangheon,

  thanks for your review, and sorry for the late answer - I have been
travelling lately...

On Thu, 2017-11-09 at 15:00 -0800, sangheon.kim wrote:

> Hi Thomas,
>
> On 11/03/2017 09:28 AM, Thomas Schatzl wrote:
> > Hi,
> >
> >    can I have reviews for this change that makes refinement threads
> > behave the same as the other thread groups we have in G1?
> >
> > I.e. with UseDynamicNumberOfGCThreads enabled (which is off by
> > default) they are created lazily.
> >
> > This helps a little bit further with startup/footprint benchmarks
> > (if
> > enabled).
> >
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8190426
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8190426/webrev/
> > Testing:
> > hs-tier1+2
>
> I like this idea, thank you for improving this.
>
> I have some comments.

I think I addressed all your concerns. Some of them were due to me
being confused about which memory allocations cause VM exit, and which
don't. I hope I got that right now.

I also changed the behavior of the thread startup to be more similar to
the worker threads, namely:
- always start one refinement thread at startup
- support InjectGCWorkerCreationFailure (which is tested by our tests
btw)
- make error messages and failure modes more similar to the ones for
the work gangs.

Even with that we want to look at JDK-8191082 for better specifying the
expected behavior.

Webrevs:
http://cr.openjdk.java.net/~tschatzl/8190426/webrev.0_to_1/ (diff)
http://cr.openjdk.java.net/~tschatzl/8190426/webrev.1/ (full)
Testing:
hs-tier1+2

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR (M): 8190426: Lazily initialize refinement threads with UseDynamicNumberOfGCThreads

Thomas Schatzl
Hi all,

  Stefan Johansson found another issue: in the original code, before
activating a thread it checked whether that thread had already been
activated. The changes did not do that.

Here's a new webrev:

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

Testing:
hs tier1+2

Thanks,
  Thomas


On Mon, 2017-11-20 at 12:26 +0100, Thomas Schatzl wrote:

> Hi Sangheon,
>
>   thanks for your review, and sorry for the late answer - I have been
> travelling lately...
>
> On Thu, 2017-11-09 at 15:00 -0800, sangheon.kim wrote:
> > Hi Thomas,
> >
> > On 11/03/2017 09:28 AM, Thomas Schatzl wrote:
> > > Hi,
> > >
> > >    can I have reviews for this change that makes refinement
> > > threads
> > > behave the same as the other thread groups we have in G1?
> > >
> > > I.e. with UseDynamicNumberOfGCThreads enabled (which is off by
> > > default) they are created lazily.
> > >
> > > This helps a little bit further with startup/footprint benchmarks
> > > (if
> > > enabled).
> > >
> > > CR:
> > > https://bugs.openjdk.java.net/browse/JDK-8190426
> > > Webrev:
> > > http://cr.openjdk.java.net/~tschatzl/8190426/webrev/
> > > Testing:
> > > hs-tier1+2
> >
> > I like this idea, thank you for improving this.
> >
> > I have some comments.
>
> I think I addressed all your concerns. Some of them were due to me
> being confused about which memory allocations cause VM exit, and
> which
> don't. I hope I got that right now.
>
> I also changed the behavior of the thread startup to be more similar
> to
> the worker threads, namely:
> - always start one refinement thread at startup
> - support InjectGCWorkerCreationFailure (which is tested by our tests
> btw)
> - make error messages and failure modes more similar to the ones for
> the work gangs.
>
> Even with that we want to look at JDK-8191082 for better specifying
> the
> expected behavior.
>
> Webrevs:
> http://cr.openjdk.java.net/~tschatzl/8190426/webrev.0_to_1/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8190426/webrev.1/ (full)
> Testing:
> hs-tier1+2
>
> Thanks,
>   Thomas
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (M): 8190426: Lazily initialize refinement threads with UseDynamicNumberOfGCThreads

sangheon.kim@oracle.com
Hi Thomas,

On 11/20/2017 10:17 AM, Thomas Schatzl wrote:

> Hi all,
>
>    Stefan Johansson found another issue: in the original code, before
> activating a thread it checked whether that thread had already been
> activated. The changes did not do that.
>
> Here's a new webrev:
>
> http://cr.openjdk.java.net/~tschatzl/8190426/webrev.1_to_2/  (diff)
> http://cr.openjdk.java.net/~tschatzl/8190426/webrev.2/  (full)
webrev.2 looks good to me.

And thank you for addressing my comments on webrev.1.
I agree we can improve lazily allocating threads with JDK-8191082: Unify
handling of thread sets when allocating them lazily.

Thanks,
Sangheon


>
> Testing:
> hs tier1+2
>
> Thanks,
>    Thomas
>
>
> On Mon, 2017-11-20 at 12:26 +0100, Thomas Schatzl wrote:
>> Hi Sangheon,
>>
>>    thanks for your review, and sorry for the late answer - I have been
>> travelling lately...
>>
>> On Thu, 2017-11-09 at 15:00 -0800, sangheon.kim wrote:
>>> Hi Thomas,
>>>
>>> On 11/03/2017 09:28 AM, Thomas Schatzl wrote:
>>>> Hi,
>>>>
>>>>     can I have reviews for this change that makes refinement
>>>> threads
>>>> behave the same as the other thread groups we have in G1?
>>>>
>>>> I.e. with UseDynamicNumberOfGCThreads enabled (which is off by
>>>> default) they are created lazily.
>>>>
>>>> This helps a little bit further with startup/footprint benchmarks
>>>> (if
>>>> enabled).
>>>>
>>>> CR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8190426
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~tschatzl/8190426/webrev/
>>>> Testing:
>>>> hs-tier1+2
>>> I like this idea, thank you for improving this.
>>>
>>> I have some comments.
>> I think I addressed all your concerns. Some of them were due to me
>> being confused about which memory allocations cause VM exit, and
>> which
>> don't. I hope I got that right now.
>>
>> I also changed the behavior of the thread startup to be more similar
>> to
>> the worker threads, namely:
>> - always start one refinement thread at startup
>> - support InjectGCWorkerCreationFailure (which is tested by our tests
>> btw)
>> - make error messages and failure modes more similar to the ones for
>> the work gangs.
>>
>> Even with that we want to look at JDK-8191082 for better specifying
>> the
>> expected behavior.
>>
>> Webrevs:
>> http://cr.openjdk.java.net/~tschatzl/8190426/webrev.0_to_1/  (diff)
>> http://cr.openjdk.java.net/~tschatzl/8190426/webrev.1/  (full)
>> Testing:
>> hs-tier1+2
>>
>> Thanks,
>>    Thomas
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (M): 8190426: Lazily initialize refinement threads with UseDynamicNumberOfGCThreads

Thomas Schatzl
Hi Sangheon,

On Tue, 2017-11-21 at 11:56 -0800, sangheon.kim wrote:

> Hi Thomas,
>
> On 11/20/2017 10:17 AM, Thomas Schatzl wrote:
> > Hi all,
> >
> >    Stefan Johansson found another issue: in the original code,
> > beforeactivating a thread it checked whether that thread had
> > already been activated. The changes did not do that.
> >
> > Here's a new webrev:
> >
> > http://cr.openjdk.java.net/~tschatzl/8190426/webrev.1_to_2//  (diff
> > )
> > http://cr.openjdk.java.net/~tschatzl/8190426/webrev.2//  (full)
>
> webrev.2 looks good to me.
>
> And thank you for addressing my comments on webrev.1.
> I agree we can improve lazily allocating threads with JDK-8191082:
> Unify handling of thread sets when allocating them lazily.
>

Thanks for your review,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR (M): 8190426: Lazily initialize refinement threads with UseDynamicNumberOfGCThreads

Stefan Johansson
In reply to this post by sangheon.kim@oracle.com


On 2017-11-21 20:56, sangheon.kim wrote:

> Hi Thomas,
>
> On 11/20/2017 10:17 AM, Thomas Schatzl wrote:
>> Hi all,
>>
>>    Stefan Johansson found another issue: in the original code, before
>> activating a thread it checked whether that thread had already been
>> activated. The changes did not do that.
>>
>> Here's a new webrev:
>>
>> http://cr.openjdk.java.net/~tschatzl/8190426/webrev.1_to_2/ (diff)
>> http://cr.openjdk.java.net/~tschatzl/8190426/webrev.2/  (full)
> webrev.2 looks good to me.
>
> And thank you for addressing my comments on webrev.1.
> I agree we can improve lazily allocating threads with JDK-8191082:
> Unify handling of thread sets when allocating them lazily.
>
Looks good Thomas,
Stefan

> Thanks,
> Sangheon
>
>
>>
>> Testing:
>> hs tier1+2
>>
>> Thanks,
>>    Thomas
>>
>>
>> On Mon, 2017-11-20 at 12:26 +0100, Thomas Schatzl wrote:
>>> Hi Sangheon,
>>>
>>>    thanks for your review, and sorry for the late answer - I have been
>>> travelling lately...
>>>
>>> On Thu, 2017-11-09 at 15:00 -0800, sangheon.kim wrote:
>>>> Hi Thomas,
>>>>
>>>> On 11/03/2017 09:28 AM, Thomas Schatzl wrote:
>>>>> Hi,
>>>>>
>>>>>     can I have reviews for this change that makes refinement
>>>>> threads
>>>>> behave the same as the other thread groups we have in G1?
>>>>>
>>>>> I.e. with UseDynamicNumberOfGCThreads enabled (which is off by
>>>>> default) they are created lazily.
>>>>>
>>>>> This helps a little bit further with startup/footprint benchmarks
>>>>> (if
>>>>> enabled).
>>>>>
>>>>> CR:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8190426
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~tschatzl/8190426/webrev/
>>>>> Testing:
>>>>> hs-tier1+2
>>>> I like this idea, thank you for improving this.
>>>>
>>>> I have some comments.
>>> I think I addressed all your concerns. Some of them were due to me
>>> being confused about which memory allocations cause VM exit, and
>>> which
>>> don't. I hope I got that right now.
>>>
>>> I also changed the behavior of the thread startup to be more similar
>>> to
>>> the worker threads, namely:
>>> - always start one refinement thread at startup
>>> - support InjectGCWorkerCreationFailure (which is tested by our tests
>>> btw)
>>> - make error messages and failure modes more similar to the ones for
>>> the work gangs.
>>>
>>> Even with that we want to look at JDK-8191082 for better specifying
>>> the
>>> expected behavior.
>>>
>>> Webrevs:
>>> http://cr.openjdk.java.net/~tschatzl/8190426/webrev.0_to_1/ (diff)
>>> http://cr.openjdk.java.net/~tschatzl/8190426/webrev.1/  (full)
>>> Testing:
>>> hs-tier1+2
>>>
>>> Thanks,
>>>    Thomas
>>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (M): 8190426: Lazily initialize refinement threads with UseDynamicNumberOfGCThreads

Thomas Schatzl
Hi Stefan,

On Wed, 2017-11-22 at 17:34 +0100, Stefan Johansson wrote:

>
> On 2017-11-21 20:56, sangheon.kim wrote:
> > Hi Thomas,
> >
> > On 11/20/2017 10:17 AM, Thomas Schatzl wrote:
> > > Hi all,
> > >
> > >    Stefan Johansson found another issue: in the original code,
> > > before activating a thread it checked whether that thread had
> > > already been activated. The changes did not do that.
> > > [...]
> >
> > webrev.2 looks good to me.
> >
> > And thank you for addressing my comments on webrev.1.
> > I agree we can improve lazily allocating threads with JDK-8191082:
> > Unify handling of thread sets when allocating them lazily.
> >
>
> Looks good Thomas,
> Stefan

Thanks for your review,
  Thomas