RFR: 8260019: Move some Thread subtypes out of thread.hpp

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

RFR: 8260019: Move some Thread subtypes out of thread.hpp

Ioi Lam-2
thread.hpp is 2133 lines long and is included by about 800 out of 1000 HotSpot .o files. It also pulls in many other header files.

Many of the Thread subtypes are infrequently used. This RFE move the easy ones to compilerThread.hpp and nonJavaThread.hpp. This reduces thread.hpp to 1888 lines.

(I hope to do more refactoring of thread.hpp in future RFEs).

Tested with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero.

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

Commit messages:
 - 8260019: Move some Thread subtypes out of thread.hpp

Changes: https://git.openjdk.java.net/jdk/pull/2390/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2390&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8260019
  Stats: 1392 lines in 18 files changed: 760 ins; 622 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2390.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2390/head:pull/2390

PR: https://git.openjdk.java.net/jdk/pull/2390
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260019: Move some Thread subtypes out of thread.hpp

David Holmes-2
On Wed, 3 Feb 2021 22:55:56 GMT, Ioi Lam <[hidden email]> wrote:

> thread.hpp is 2133 lines long and is included by about 800 out of 1000 HotSpot .o files. It also pulls in many other header files.
>
> Many of the Thread subtypes are infrequently used. This RFE move the easy ones to compilerThread.hpp and nonJavaThread.hpp. This reduces thread.hpp to 1888 lines.
>
> (I hope to do more refactoring of thread.hpp in future RFEs).
>
> Tested with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero.

Hi Ioi,

I'm assuming the code is basically just a cut'n'paste with few actual changes needed thereafter - though I did spot the need to make the thread_entry member functions.

I'm not sure about the choice of new file names given they contain code for multiple thread types. I don't have great names to suggest as they stand. I wouldn't object to the sweeper and watcher threads getting their own files - but don't insist at this time.

I can't decide whether including both fooThread.hpp and thread.hpp in the same cpp file is good code hygiene or a complete waste of time. It is kind of obvious that fooThread.hpp must include thread.hpp.

Overall I come down on the side of approval. :)

Thanks,
David

src/hotspot/share/compiler/compilerThread.hpp line 2:

> 1: /*
> 2:  * Copyright (c) 1997, 2021, Oracle and/or its affiliates. All rights reserved.

Should a new file only have a single copyright year? I'm not sure what the rule is about splitting existing files but you use a single year in the new cpp files.

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

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2390
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260019: Move some Thread subtypes out of thread.hpp

Coleen Phillimore-3
In reply to this post by Ioi Lam-2
On Wed, 3 Feb 2021 22:55:56 GMT, Ioi Lam <[hidden email]> wrote:

> thread.hpp is 2133 lines long and is included by about 800 out of 1000 HotSpot .o files. It also pulls in many other header files.
>
> Many of the Thread subtypes are infrequently used. This RFE move the easy ones to compilerThread.hpp and nonJavaThread.hpp. This reduces thread.hpp to 1888 lines.
>
> (I hope to do more refactoring of thread.hpp in future RFEs).
>
> Tested with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero.

I like this a lot.

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

Marked as reviewed by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2390
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260019: Move some Thread subtypes out of thread.hpp

Coleen Phillimore-3
In reply to this post by David Holmes-2
On Thu, 4 Feb 2021 06:19:21 GMT, David Holmes <[hidden email]> wrote:

>> thread.hpp is 2133 lines long and is included by about 800 out of 1000 HotSpot .o files. It also pulls in many other header files.
>>
>> Many of the Thread subtypes are infrequently used. This RFE move the easy ones to compilerThread.hpp and nonJavaThread.hpp. This reduces thread.hpp to 1888 lines.
>>
>> (I hope to do more refactoring of thread.hpp in future RFEs).
>>
>> Tested with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero.
>
> src/hotspot/share/compiler/compilerThread.hpp line 2:
>
>> 1: /*
>> 2:  * Copyright (c) 1997, 2021, Oracle and/or its affiliates. All rights reserved.
>
> Should a new file only have a single copyright year? I'm not sure what the rule is about splitting existing files but you use a single year in the new cpp files.

I think the rule has been: if it's a new file, it gets a new year.

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

PR: https://git.openjdk.java.net/jdk/pull/2390
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260019: Move some Thread subtypes out of thread.hpp [v2]

Ioi Lam-2
In reply to this post by Ioi Lam-2
> thread.hpp is 2133 lines long and is included by about 800 out of 1000 HotSpot .o files. It also pulls in many other header files.
>
> Many of the Thread subtypes are infrequently used. This RFE move the easy ones to compilerThread.hpp and nonJavaThread.hpp. This reduces thread.hpp to 1888 lines.
>
> (I hope to do more refactoring of thread.hpp in future RFEs).
>
> Tested with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero.

Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:

 - fixed merge
 - Merge branch 'master' into 8260019-move-some-subtypes-out-of-thread-hpp
 - fixed copyright year
 - 8260019: Move some Thread subtypes out of thread.hpp

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

Changes: https://git.openjdk.java.net/jdk/pull/2390/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2390&range=01
  Stats: 1392 lines in 19 files changed: 762 ins; 620 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2390.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2390/head:pull/2390

PR: https://git.openjdk.java.net/jdk/pull/2390
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260019: Move some Thread subtypes out of thread.hpp [v2]

Ioi Lam-2
In reply to this post by Coleen Phillimore-3
On Thu, 4 Feb 2021 13:46:28 GMT, Coleen Phillimore <[hidden email]> wrote:

>> src/hotspot/share/compiler/compilerThread.hpp line 2:
>>
>>> 1: /*
>>> 2:  * Copyright (c) 1997, 2021, Oracle and/or its affiliates. All rights reserved.
>>
>> Should a new file only have a single copyright year? I'm not sure what the rule is about splitting existing files but you use a single year in the new cpp files.
>
> I think the rule has been: if it's a new file, it gets a new year.

Hi David, you're correct that I just cut-and-pasted the code; the only exception was I made the `thread_entry` functions member functions.

I don't have a better idea for the file names, either. I'll keep them as is in the patch. Maybe someone else with a better sense for naming could fix our file names in the future.

I fixed the copyright year.

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

PR: https://git.openjdk.java.net/jdk/pull/2390
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260019: Move some Thread subtypes out of thread.hpp [v2]

Ioi Lam-2
In reply to this post by Coleen Phillimore-3
On Thu, 4 Feb 2021 13:49:33 GMT, Coleen Phillimore <[hidden email]> wrote:

>> Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:
>>
>>  - fixed merge
>>  - Merge branch 'master' into 8260019-move-some-subtypes-out-of-thread-hpp
>>  - fixed copyright year
>>  - 8260019: Move some Thread subtypes out of thread.hpp
>
> I like this a lot.

Thanks @coleenp and @dholmes-ora for the review.

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

PR: https://git.openjdk.java.net/jdk/pull/2390
Reply | Threaded
Open this post in threaded view
|

Integrated: 8260019: Move some Thread subtypes out of thread.hpp

Ioi Lam-2
In reply to this post by Ioi Lam-2
On Wed, 3 Feb 2021 22:55:56 GMT, Ioi Lam <[hidden email]> wrote:

> thread.hpp is 2133 lines long and is included by about 800 out of 1000 HotSpot .o files. It also pulls in many other header files.
>
> Many of the Thread subtypes are infrequently used. This RFE move the easy ones to compilerThread.hpp and nonJavaThread.hpp. This reduces thread.hpp to 1888 lines.
>
> (I hope to do more refactoring of thread.hpp in future RFEs).
>
> Tested with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero.

This pull request has now been integrated.

Changeset: c5bb1092
Author:    Ioi Lam <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/c5bb1092
Stats:     1392 lines in 19 files changed: 762 ins; 620 del; 10 mod

8260019: Move some Thread subtypes out of thread.hpp

Reviewed-by: dholmes, coleenp

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

PR: https://git.openjdk.java.net/jdk/pull/2390
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260019: Move some Thread subtypes out of thread.hpp [v2]

Stefan Karlsson-3
In reply to this post by Ioi Lam-2
On Fri, 5 Feb 2021 02:57:20 GMT, Ioi Lam <[hidden email]> wrote:

>> I think the rule has been: if it's a new file, it gets a new year.
>
> Hi David, you're correct that I just cut-and-pasted the code; the only exception was I made the `thread_entry` functions member functions.
>
> I don't have a better idea for the file names, either. I'll keep them as is in the patch. Maybe someone else with a better sense for naming could fix our file names in the future.
>
> I fixed the copyright year.

> I think the rule has been: if it's a new file, it gets a new year.

My understanding is that if code has been copied, then the old copyright dates should be kept.

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

PR: https://git.openjdk.java.net/jdk/pull/2390