RFR (S): 8189673: Consistent naming of concurrent threads, tasks and related identifiers

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

RFR (S): 8189673: Consistent naming of concurrent threads, tasks and related identifiers

Thomas Schatzl
Hi,

  please review the following change that tries to consistently name
concurrent workers and related members to not contain any of "marking",
"mark", "parallel" or something similar, to use identifiers related to
"concurrent workers".

The reason is that G1's concurrent worker threads for a long time now
do more than strictly only "marking", but are also used for other, but
typically related tasks.

Also, do not use "parallel" to avoid confusion with the work gang used
during stw pauses. I think by now in the GC area the terms "concurrent"
and "parallel" have a sufficiently distinct meaning to be useful.

Also change _max_worker_id to the more specific _max_num_tasks to not
clash with the new naming. I.e. that it does not appear to be related
to the concurrent workers.

CR:
https://bugs.openjdk.java.net/browse/JDK-8189673
Webrev:
http://cr.openjdk.java.net/~tschatzl/8189673/webrev/
Testing:
jprt

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

RE: RFR (S): 8189673: Consistent naming of concurrent threads, tasks and related identifiers

White, Derek
Hi Thomas,

That looks better!

Related to your goal:
The files are called g1ConcurrentMark.[cpp|hpp]. That's fine for now, but should there be a clarifying comment in the .hpp that class G1ConcurrentMark handles concurrent marking as well as X, Y, and Z?

Also a nit, but "marking_thread_num = scale_mark_threads(ParallelGCThreads)" sounds a little too "marky" after your changes.

Thanks,

- Derek

> -----Original Message-----
> From: hotspot-gc-dev [mailto:[hidden email]]
> On Behalf Of Thomas Schatzl
> Sent: Monday, October 23, 2017 11:55 AM
> To: [hidden email]
> Subject: RFR (S): 8189673: Consistent naming of concurrent threads, tasks
> and related identifiers
>
> Hi,
>
>   please review the following change that tries to consistently name
> concurrent workers and related members to not contain any of "marking",
> "mark", "parallel" or something similar, to use identifiers related to
> "concurrent workers".
>
> The reason is that G1's concurrent worker threads for a long time now do
> more than strictly only "marking", but are also used for other, but typically
> related tasks.
>
> Also, do not use "parallel" to avoid confusion with the work gang used
> during stw pauses. I think by now in the GC area the terms "concurrent"
> and "parallel" have a sufficiently distinct meaning to be useful.
>
> Also change _max_worker_id to the more specific _max_num_tasks to not
> clash with the new naming. I.e. that it does not appear to be related to the
> concurrent workers.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8189673
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8189673/webrev/
> Testing:
> jprt
>
> Thanks,
>   Thomas
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8189673: Consistent naming of concurrent threads, tasks and related identifiers

Thomas Schatzl
Hi,

On Mon, 2017-10-23 at 21:53 +0000, White, Derek wrote:
> Hi Thomas,
>
> That looks better!
>
> Related to your goal:
> The files are called g1ConcurrentMark.[cpp|hpp]. That's fine for now,
> but should there be a clarifying comment in the .hpp that class
> G1ConcurrentMark handles concurrent marking as well as X, Y, and Z?

I added a comment that it provides data structures for the concurrent
cycle without going into detail.
There may be significant changes soon ;)

> Also a nit, but "marking_thread_num =
> scale_mark_threads(ParallelGCThreads)" sounds a little too "marky"
> after your changes.

Fixed that, thanks.

New webrev:
http://cr.openjdk.java.net/~tschatzl/8189673/webrev.1 (full)
http://cr.openjdk.java.net/~tschatzl/8189673/webrev.0_to_1 (diff)
Testing:
local compilation

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

RE: RFR (S): 8189673: Consistent naming of concurrent threads, tasks and related identifiers

White, Derek
Thanks Thomas,

Thumbs up from me!

 - Derek

> -----Original Message-----
> From: Thomas Schatzl [mailto:[hidden email]]
> Sent: Tuesday, October 24, 2017 4:03 AM
> To: White, Derek <[hidden email]>; hotspot-gc-
> [hidden email]
> Subject: Re: RFR (S): 8189673: Consistent naming of concurrent threads,
> tasks and related identifiers
>
> Hi,
>
> On Mon, 2017-10-23 at 21:53 +0000, White, Derek wrote:
> > Hi Thomas,
> >
> > That looks better!
> >
> > Related to your goal:
> > The files are called g1ConcurrentMark.[cpp|hpp]. That's fine for now,
> > but should there be a clarifying comment in the .hpp that class
> > G1ConcurrentMark handles concurrent marking as well as X, Y, and Z?
>
> I added a comment that it provides data structures for the concurrent cycle
> without going into detail.
> There may be significant changes soon ;)
>
> > Also a nit, but "marking_thread_num =
> > scale_mark_threads(ParallelGCThreads)" sounds a little too "marky"
> > after your changes.
>
> Fixed that, thanks.
>
> New webrev:
> http://cr.openjdk.java.net/~tschatzl/8189673/webrev.1 (full)
> http://cr.openjdk.java.net/~tschatzl/8189673/webrev.0_to_1 (diff)
> Testing:
> local compilation
>
> Thanks,
>   Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8189673: Consistent naming of concurrent threads, tasks and related identifiers

Stefan Johansson
In reply to this post by Thomas Schatzl
Looks good,
StefanJ

On 2017-10-24 10:02, Thomas Schatzl wrote:

> Hi,
>
> On Mon, 2017-10-23 at 21:53 +0000, White, Derek wrote:
>> Hi Thomas,
>>
>> That looks better!
>>
>> Related to your goal:
>> The files are called g1ConcurrentMark.[cpp|hpp]. That's fine for now,
>> but should there be a clarifying comment in the .hpp that class
>> G1ConcurrentMark handles concurrent marking as well as X, Y, and Z?
> I added a comment that it provides data structures for the concurrent
> cycle without going into detail.
> There may be significant changes soon ;)
>
>> Also a nit, but "marking_thread_num =
>> scale_mark_threads(ParallelGCThreads)" sounds a little too "marky"
>> after your changes.
> Fixed that, thanks.
>
> New webrev:
> http://cr.openjdk.java.net/~tschatzl/8189673/webrev.1 (full)
> http://cr.openjdk.java.net/~tschatzl/8189673/webrev.0_to_1 (diff)
> Testing:
> local compilation
>
> Thanks,
>    Thomas
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8189673: Consistent naming of concurrent threads, tasks and related identifiers

Thomas Schatzl
Hi Stefan, Derek,

  thanks for your reviews.

Thomas