RFR: 8261125: Move VM_Operation to vmOperation.hpp

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

RFR: 8261125: Move VM_Operation to vmOperation.hpp

Ioi Lam-2
vmOperations.hpp declares the VM_Operation class, as well as a hodge podge of subclasses such as VM_ForceSafepoint, VM_DeoptimizeFrame.

Out of the 1000 hotspot .o files, about 680 include vmOperations.hpp (mostly transitively). In most cases, they just need to use the VM_Operation class.

So we should move VM_Operation to its own header: vmOperation.hpp (no "s").

After the refactoring, vmOperations.hpp is included only 64 times. The inclusion count of threadSMR.hpp is also reduced from 687 to 99. HotSpot build time is improved by about 0.4%.

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

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

Commit messages:
 - 8261125: Move VM_Operation to vmOperation.hpp

Changes: https://git.openjdk.java.net/jdk/pull/2398/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2398&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261125
  Stats: 347 lines in 24 files changed: 189 ins; 142 del; 16 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2398.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2398/head:pull/2398

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

Re: RFR: 8261125: Move VM_Operation to vmOperation.hpp

Coleen Phillimore-3
On Thu, 4 Feb 2021 05:38:49 GMT, Ioi Lam <[hidden email]> wrote:

> vmOperations.hpp declares the VM_Operation class, as well as a hodge podge of subclasses such as VM_ForceSafepoint, VM_DeoptimizeFrame.
>
> Out of the 1000 hotspot .o files, about 680 include vmOperations.hpp (mostly transitively). In most cases, they just need to use the VM_Operation class.
>
> So we should move VM_Operation to its own header: vmOperation.hpp (no "s").
>
> After the refactoring, vmOperations.hpp is included only 64 times. The inclusion count of threadSMR.hpp is also reduced from 687 to 99. HotSpot build time is improved by about 0.4%.
>
> Tested with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero.

Ok!

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

Marked as reviewed by coleenp (Reviewer).

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

Re: RFR: 8261125: Move VM_Operation to vmOperation.hpp

Thomas Stuefe
In reply to this post by Ioi Lam-2
On Thu, 4 Feb 2021 05:38:49 GMT, Ioi Lam <[hidden email]> wrote:

> vmOperations.hpp declares the VM_Operation class, as well as a hodge podge of subclasses such as VM_ForceSafepoint, VM_DeoptimizeFrame.
>
> Out of the 1000 hotspot .o files, about 680 include vmOperations.hpp (mostly transitively). In most cases, they just need to use the VM_Operation class.
>
> So we should move VM_Operation to its own header: vmOperation.hpp (no "s").
>
> After the refactoring, vmOperations.hpp is included only 64 times. The inclusion count of threadSMR.hpp is also reduced from 687 to 99. HotSpot build time is improved by about 0.4%.
>
> 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 like all these include cleanups! How do you find these, do you analyze the include tree?

I think vmOperation vs vmOperations could be confusing. But have no immediate better idea. If others are fine with it, I am too.

Can you please enable GA so we see that our weirder platforms build?

Otherwise, looks good.

..Thomas

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

Marked as reviewed by stuefe (Reviewer).

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

Re: RFR: 8261125: Move VM_Operation to vmOperation.hpp

Coleen Phillimore-2


On 2/4/21 12:18 PM, Thomas Stuefe wrote:

> On Thu, 4 Feb 2021 05:38:49 GMT, Ioi Lam <[hidden email]> wrote:
>
>> vmOperations.hpp declares the VM_Operation class, as well as a hodge podge of subclasses such as VM_ForceSafepoint, VM_DeoptimizeFrame.
>>
>> Out of the 1000 hotspot .o files, about 680 include vmOperations.hpp (mostly transitively). In most cases, they just need to use the VM_Operation class.
>>
>> So we should move VM_Operation to its own header: vmOperation.hpp (no "s").
>>
>> After the refactoring, vmOperations.hpp is included only 64 times. The inclusion count of threadSMR.hpp is also reduced from 687 to 99. HotSpot build time is improved by about 0.4%.
>>
>> 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 like all these include cleanups! How do you find these, do you analyze the include tree?
>
> I think vmOperation vs vmOperations could be confusing. But have no immediate better idea. If others are fine with it, I am too.

I thought briefly that I will be annoyed if I keep typing the extra 's'
when editing the file, but I don't edit that file very much, so I'm fine
with it.
Coleen

>
> Can you please enable GA so we see that our weirder platforms build?
>
> Otherwise, looks good.
>
> ..Thomas
>
> -------------
>
> Marked as reviewed by stuefe (Reviewer).
>
> PR: https://git.openjdk.java.net/jdk/pull/2398

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261125: Move VM_Operation to vmOperation.hpp

Daniel D.Daugherty
In reply to this post by Thomas Stuefe
On Thu, 4 Feb 2021 17:15:37 GMT, Thomas Stuefe <[hidden email]> wrote:

>> vmOperations.hpp declares the VM_Operation class, as well as a hodge podge of subclasses such as VM_ForceSafepoint, VM_DeoptimizeFrame.
>>
>> Out of the 1000 hotspot .o files, about 680 include vmOperations.hpp (mostly transitively). In most cases, they just need to use the VM_Operation class.
>>
>> So we should move VM_Operation to its own header: vmOperation.hpp (no "s").
>>
>> After the refactoring, vmOperations.hpp is included only 64 times. The inclusion count of threadSMR.hpp is also reduced from 687 to 99. HotSpot build time is improved by about 0.4%.
>>
>> 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 like all these include cleanups! How do you find these, do you analyze the include tree?
>
> I think vmOperation vs vmOperations could be confusing. But have no immediate better idea. If others are fine with it, I am too.
>
> Can you please enable GA so we see that our weirder platforms build?
>
> Otherwise, looks good.
>
> ..Thomas

A drive-by comment...

The class is named VM_Operation, but the file is named vmOperation.hpp.
The missing '_' in the file name is a bit of a disconnect, but there are worse
in the code base...

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

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

Re: RFR: 8261125: Move VM_Operation to vmOperation.hpp

David Holmes-2
In reply to this post by Ioi Lam-2
On Thu, 4 Feb 2021 05:38:49 GMT, Ioi Lam <[hidden email]> wrote:

> vmOperations.hpp declares the VM_Operation class, as well as a hodge podge of subclasses such as VM_ForceSafepoint, VM_DeoptimizeFrame.
>
> Out of the 1000 hotspot .o files, about 680 include vmOperations.hpp (mostly transitively). In most cases, they just need to use the VM_Operation class.
>
> So we should move VM_Operation to its own header: vmOperation.hpp (no "s").
>
> After the refactoring, vmOperations.hpp is included only 64 times. The inclusion count of threadSMR.hpp is also reduced from 687 to 99. HotSpot build time is improved by about 0.4%.
>
> Tested with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero.

Hi Ioi,

The distinction between vmOperation versus vmOperations is far too subtle. Perhaps as Dan implied vm_Operation.hpp or VM_Operation.hpp (though that breaks normal - odd - naming convention).

I assume most files that include vmOperation.hpp are those that define VM_Operation subclasses?

Thanks,
David

src/hotspot/share/runtime/vmOperation.hpp line 2:

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

New file so single copyright year.

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

Changes requested by dholmes (Reviewer).

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

Re: RFR: 8261125: Move VM_Operation to vmOperation.hpp

Ioi Lam-2
In reply to this post by Thomas Stuefe
On Thu, 4 Feb 2021 17:15:37 GMT, Thomas Stuefe <[hidden email]> wrote:

> I like all these include cleanups! How do you find these, do you analyze the include tree?

I have a few hand-rolled tools for identifying what header files are easy/beneficial to refactor. See

https://github.com/iklam/tools/tree/main/headers

> I think vmOperation vs vmOperations could be confusing. But have no immediate better idea. If others are fine with it, I am too.
>
> Can you please enable GA so we see that our weirder platforms build?

I think I finally re-enabled the GitHub actions properly for my PRs. Let's see if they get run for my next push to this PR :-)

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

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

Re: RFR: 8261125: Move VM_Operation to vmOperation.hpp

Ioi Lam-2
In reply to this post by David Holmes-2
On Thu, 4 Feb 2021 22:24:38 GMT, David Holmes <[hidden email]> wrote:

> Hi Ioi,
>
> The distinction between vmOperation versus vmOperations is far too subtle. Perhaps as Dan implied vm_Operation.hpp or VM_Operation.hpp (though that breaks normal - odd - naming convention).

How about this:

- runtime/vmOperation.hpp   --- (new file) this is the file that declares VM_Operation
- runtime/commonVMOperations.hpp -- (renamed from vmOperations.hpp) these are the VM_Operation subclasses that no one cares to organize properly :-)

this will be kinda consistent with these existing files:

- gc/g1/g1VMOperations.hpp
- gc/g1/g1VMOperations.cpp
- gc/shenandoah/shenandoahVMOperations.cpp
- gc/shenandoah/shenandoahVMOperations.hpp
- gc/shared/gcVMOperations.cpp
- gc/shared/gcVMOperations.hpp
- gc/parallel/psVMOperations.cpp
- gc/parallel/psVMOperations.hpp

(I should also add a new vmOperation.cpp, and rename vmOperations.cpp to commonVMOperations.cpp)

BTW, I need to refactor VM_Exit into its own file. It's used by the `JVM_LEAF` macro in interfaceSupport.inline.hpp, but I don't want to pull in all the other "common" operations in there.  I am thinking of calling it vmExit.hpp (since exitVMOperation.hpp doesn't really look good).

> I assume most files that include vmOperation.hpp are those that define VM_Operation subclasses?

Yes, but there are also files that use the `VM_Operation::VMOp_Type` type, notably safepoint.hpp.

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

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

Re: RFR: 8261125: Move VM_Operation to vmOperation.hpp

David Holmes
On 5/02/2021 3:53 pm, Ioi Lam wrote:

> On Thu, 4 Feb 2021 22:24:38 GMT, David Holmes <[hidden email]> wrote:
>
>> Hi Ioi,
>>
>> The distinction between vmOperation versus vmOperations is far too subtle. Perhaps as Dan implied vm_Operation.hpp or VM_Operation.hpp (though that breaks normal - odd - naming convention).
>
> How about this:
>
> - runtime/vmOperation.hpp   --- (new file) this is the file that declares VM_Operation
> - runtime/commonVMOperations.hpp -- (renamed from vmOperations.hpp) these are the VM_Operation subclasses that no one cares to organize properly :-)
>
> this will be kinda consistent with these existing files:
>
> - gc/g1/g1VMOperations.hpp
> - gc/g1/g1VMOperations.cpp
> - gc/shenandoah/shenandoahVMOperations.cpp
> - gc/shenandoah/shenandoahVMOperations.hpp
> - gc/shared/gcVMOperations.cpp
> - gc/shared/gcVMOperations.hpp
> - gc/parallel/psVMOperations.cpp
> - gc/parallel/psVMOperations.hpp
>
> (I should also add a new vmOperation.cpp, and rename vmOperations.cpp to commonVMOperations.cpp)

Okay.

> BTW, I need to refactor VM_Exit into its own file. It's used by the `JVM_LEAF` macro in interfaceSupport.inline.hpp, but I don't want to pull in all the other "common" operations in there.  I am thinking of calling it vmExit.hpp (since exitVMOperation.hpp doesn't really look good).

:( Does it really matter? We're going to end up with a bazillion files
at this rate and the build time improvements are not even perceptible.

David
-----

>> I assume most files that include vmOperation.hpp are those that define VM_Operation subclasses?
>
> Yes, but there are also files that use the `VM_Operation::VMOp_Type` type, notably safepoint.hpp.
>
> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/2398
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261125: Move VM_Operation to vmOperation.hpp

Coleen Phillimore-3
In reply to this post by Ioi Lam-2
On Fri, 5 Feb 2021 05:51:22 GMT, Ioi Lam <[hidden email]> wrote:

>> Hi Ioi,
>>
>> The distinction between vmOperation versus vmOperations is far too subtle. Perhaps as Dan implied vm_Operation.hpp or VM_Operation.hpp (though that breaks normal - odd - naming convention).
>>
>> I assume most files that include vmOperation.hpp are those that define VM_Operation subclasses?
>>
>> Thanks,
>> David
>
>> Hi Ioi,
>>
>> The distinction between vmOperation versus vmOperations is far too subtle. Perhaps as Dan implied vm_Operation.hpp or VM_Operation.hpp (though that breaks normal - odd - naming convention).
>
> How about this:
>
> - runtime/vmOperation.hpp   --- (new file) this is the file that declares VM_Operation
> - runtime/commonVMOperations.hpp -- (renamed from vmOperations.hpp) these are the VM_Operation subclasses that no one cares to organize properly :-)
>
> this will be kinda consistent with these existing files:
>
> - gc/g1/g1VMOperations.hpp
> - gc/g1/g1VMOperations.cpp
> - gc/shenandoah/shenandoahVMOperations.cpp
> - gc/shenandoah/shenandoahVMOperations.hpp
> - gc/shared/gcVMOperations.cpp
> - gc/shared/gcVMOperations.hpp
> - gc/parallel/psVMOperations.cpp
> - gc/parallel/psVMOperations.hpp
>
> (I should also add a new vmOperation.cpp, and rename vmOperations.cpp to commonVMOperations.cpp)
>
> BTW, I need to refactor VM_Exit into its own file. It's used by the `JVM_LEAF` macro in interfaceSupport.inline.hpp, but I don't want to pull in all the other "common" operations in there.  I am thinking of calling it vmExit.hpp (since exitVMOperation.hpp doesn't really look good).
>
>> I assume most files that include vmOperation.hpp are those that define VM_Operation subclasses?
>
> Yes, but there are also files that use the `VM_Operation::VMOp_Type` type, notably safepoint.hpp.

I'm fine with leaving vmOperations.hpp and vmOperation.hpp.  It's not a big deal.  commonVMOperations.hpp - too much noise!
I agree with David.  interfaceSupport.inline.hpp imports a lot of things so importing vmOperations.hpp is not a big deal.  vmOperations.hpp imports #include "runtime/threadSMR.hpp" otherwise it has all the same imports as interfaceSupport.inline.hpp anyway.
All these files are going to increase compilation time too.
I stand by my check mark above!

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

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

Re: RFR: 8261125: Move VM_Operation to vmOperation.hpp [v2]

Ioi Lam-2
In reply to this post by Ioi Lam-2
> vmOperations.hpp declares the VM_Operation class, as well as a hodge podge of subclasses such as VM_ForceSafepoint, VM_DeoptimizeFrame.
>
> Out of the 1000 hotspot .o files, about 680 include vmOperations.hpp (mostly transitively). In most cases, they just need to use the VM_Operation class.
>
> So we should move VM_Operation to its own header: vmOperation.hpp (no "s").
>
> After the refactoring, vmOperations.hpp is included only 64 times. The inclusion count of threadSMR.hpp is also reduced from 687 to 99. HotSpot build time is improved by about 0.4%.
>
> 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 incrementally with one additional commit since the last revision:

  interfaceSupport.inline.hpp needs VM_Exit from vmOperations.hpp for JVM_LEAF macro

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2398/files
  - new: https://git.openjdk.java.net/jdk/pull/2398/files/f4637f70..01633cda

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2398&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2398&range=00-01

  Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2398.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2398/head:pull/2398

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

Re: RFR: 8261125: Move VM_Operation to vmOperation.hpp [v2]

Ioi Lam-2
In reply to this post by Coleen Phillimore-3
On Fri, 5 Feb 2021 17:34:06 GMT, Coleen Phillimore <[hidden email]> wrote:

> I'm fine with leaving vmOperations.hpp and vmOperation.hpp. It's not a big deal. commonVMOperations.hpp - too much noise!
> I agree with David. interfaceSupport.inline.hpp imports a lot of things so importing vmOperations.hpp is not a big deal. vmOperations.hpp imports #include "runtime/threadSMR.hpp" otherwise it has all the same imports as interfaceSupport.inline.hpp anyway.
> All these files are going to increase compilation time too.
> I stand by my check mark above!

OK, since no one is fond of further splitting the headers, and no one has vetoed the vmOperation.hpp name, I'll keep everything as originally proposed. Will do more testing and integrate.

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

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

Re: RFR: 8261125: Move VM_Operation to vmOperation.hpp [v3]

Ioi Lam-2
In reply to this post by Ioi Lam-2
> vmOperations.hpp declares the VM_Operation class, as well as a hodge podge of subclasses such as VM_ForceSafepoint, VM_DeoptimizeFrame.
>
> Out of the 1000 hotspot .o files, about 680 include vmOperations.hpp (mostly transitively). In most cases, they just need to use the VM_Operation class.
>
> So we should move VM_Operation to its own header: vmOperation.hpp (no "s").
>
> After the refactoring, vmOperations.hpp is included only 64 times. The inclusion count of threadSMR.hpp is also reduced from 687 to 99. HotSpot build time is improved by about 0.4%.
>
> 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 incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:

 - Merge branch 'master' into 8261125-move-VM_Operation-to-vmOperation.hpp
 - interfaceSupport.inline.hpp needs VM_Exit from vmOperations.hpp for JVM_LEAF macro
 - 8261125: Move VM_Operation to vmOperation.hpp

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2398/files
  - new: https://git.openjdk.java.net/jdk/pull/2398/files/01633cda..bab79154

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2398&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2398&range=01-02

  Stats: 19114 lines in 561 files changed: 11182 ins; 5559 del; 2373 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2398.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2398/head:pull/2398

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

Re: RFR: 8261125: Move VM_Operation to vmOperation.hpp [v3]

Ioi Lam-2
In reply to this post by Ioi Lam-2
On Thu, 11 Feb 2021 03:56:44 GMT, Ioi Lam <[hidden email]> wrote:

>> I'm fine with leaving vmOperations.hpp and vmOperation.hpp.  It's not a big deal.  commonVMOperations.hpp - too much noise!
>> I agree with David.  interfaceSupport.inline.hpp imports a lot of things so importing vmOperations.hpp is not a big deal.  vmOperations.hpp imports #include "runtime/threadSMR.hpp" otherwise it has all the same imports as interfaceSupport.inline.hpp anyway.
>> All these files are going to increase compilation time too.
>> I stand by my check mark above!
>
>> I'm fine with leaving vmOperations.hpp and vmOperation.hpp. It's not a big deal. commonVMOperations.hpp - too much noise!
>> I agree with David. interfaceSupport.inline.hpp imports a lot of things so importing vmOperations.hpp is not a big deal. vmOperations.hpp imports #include "runtime/threadSMR.hpp" otherwise it has all the same imports as interfaceSupport.inline.hpp anyway.
>> All these files are going to increase compilation time too.
>> I stand by my check mark above!
>
> OK, since no one is fond of further splitting the headers, and no one has vetoed the vmOperation.hpp name, I'll keep everything as originally proposed. Will do more testing and integrate.

BTW, I found a few existing singular/plural pairs of hpp files. Admittedly I created ciSymbols.hpp recently, but the other 3 pairs have been there for quite some time.

share/ci/ciSymbol.hpp
share/ci/ciSymbols.hpp

share/interpreter/bytecode.hpp
share/interpreter/bytecodes.hpp

share/jfr/recorder/service/jfrEvent.hpp
share/jfr/jfrEvents.hpp

share/jfr/recorder/checkpoint/types/jfrType.hpp
share/jfr/utilities/jfrTypes.hpp

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

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

Re: RFR: 8261125: Move VM_Operation to vmOperation.hpp [v4]

Ioi Lam-2
In reply to this post by Ioi Lam-2
> vmOperations.hpp declares the VM_Operation class, as well as a hodge podge of subclasses such as VM_ForceSafepoint, VM_DeoptimizeFrame.
>
> Out of the 1000 hotspot .o files, about 680 include vmOperations.hpp (mostly transitively). In most cases, they just need to use the VM_Operation class.
>
> So we should move VM_Operation to its own header: vmOperation.hpp (no "s").
>
> After the refactoring, vmOperations.hpp is included only 64 times. The inclusion count of threadSMR.hpp is also reduced from 687 to 99. HotSpot build time is improved by about 0.4%.
>
> 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 incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:

 - Merge branch 'master' into 8261125-move-VM_Operation-to-vmOperation.hpp
 - Merge branch 'master' into 8261125-move-VM_Operation-to-vmOperation.hpp
 - interfaceSupport.inline.hpp needs VM_Exit from vmOperations.hpp for JVM_LEAF macro
 - 8261125: Move VM_Operation to vmOperation.hpp

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2398/files
  - new: https://git.openjdk.java.net/jdk/pull/2398/files/bab79154..5b8f1b4b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2398&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2398&range=02-03

  Stats: 8513 lines in 340 files changed: 4350 ins; 2305 del; 1858 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2398.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2398/head:pull/2398

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

Re: RFR: 8261125: Move VM_Operation to vmOperation.hpp [v4]

Ioi Lam-2
In reply to this post by Ioi Lam-2
On Thu, 11 Feb 2021 06:54:40 GMT, Ioi Lam <[hidden email]> wrote:

>>> I'm fine with leaving vmOperations.hpp and vmOperation.hpp. It's not a big deal. commonVMOperations.hpp - too much noise!
>>> I agree with David. interfaceSupport.inline.hpp imports a lot of things so importing vmOperations.hpp is not a big deal. vmOperations.hpp imports #include "runtime/threadSMR.hpp" otherwise it has all the same imports as interfaceSupport.inline.hpp anyway.
>>> All these files are going to increase compilation time too.
>>> I stand by my check mark above!
>>
>> OK, since no one is fond of further splitting the headers, and no one has vetoed the vmOperation.hpp name, I'll keep everything as originally proposed. Will do more testing and integrate.
>
> BTW, I found a few existing singular/plural pairs of hpp files. Admittedly I created ciSymbols.hpp recently, but the other 3 pairs have been there for quite some time.
>
> share/ci/ciSymbol.hpp
> share/ci/ciSymbols.hpp
>
> share/interpreter/bytecode.hpp
> share/interpreter/bytecodes.hpp
>
> share/jfr/recorder/service/jfrEvent.hpp
> share/jfr/jfrEvents.hpp
>
> share/jfr/recorder/checkpoint/types/jfrType.hpp
> share/jfr/utilities/jfrTypes.hpp

Thanks @dcubed-ojdk @coleenp @tstuefe @dholmes-ora for the review.

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

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

Integrated: 8261125: Move VM_Operation to vmOperation.hpp

Ioi Lam-2
In reply to this post by Ioi Lam-2
On Thu, 4 Feb 2021 05:38:49 GMT, Ioi Lam <[hidden email]> wrote:

> vmOperations.hpp declares the VM_Operation class, as well as a hodge podge of subclasses such as VM_ForceSafepoint, VM_DeoptimizeFrame.
>
> Out of the 1000 hotspot .o files, about 680 include vmOperations.hpp (mostly transitively). In most cases, they just need to use the VM_Operation class.
>
> So we should move VM_Operation to its own header: vmOperation.hpp (no "s").
>
> After the refactoring, vmOperations.hpp is included only 64 times. The inclusion count of threadSMR.hpp is also reduced from 687 to 99. HotSpot build time is improved by about 0.4%.
>
> 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: fc1d0321
Author:    Ioi Lam <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/fc1d0321
Stats:     345 lines in 23 files changed: 189 ins; 141 del; 15 mod

8261125: Move VM_Operation to vmOperation.hpp

Reviewed-by: coleenp, stuefe

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

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