8288537: Move Devirtualizer out of hotspot/share/memory/iterator.hpp

[iklam]

HotSpot build time has regressed quite a bit since JDK 17 (1m46s vs 2m20s on my machine). Now it's time to do some header file cleanup.

iterator.hpp contains a hodge-podge of unrelated classes. Some of these classes have dependencies on other headers (e.g., Devirtualizer depends on bitMap.hpp) that slow down C++ compilation.

This patch moves two infrequently used classes, Devirtualizer and SerializeClosure, to their own header files. This reduces the total number of C++ lines compiled for libjvm.so for about 1%.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8288537: Move Devirtualizer out of hotspot/share/memory/iterator.hpp

Reviewers

• Stefan Karlsson (@stefank - Reviewer) ⚠️ Review applies to ab986620
• Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to ba07ef2c

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9176/head:pull/9176
$ git checkout pull/9176

Update a local copy of the PR:
$ git checkout pull/9176
$ git pull https://git.openjdk.org/jdk pull/9176/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9176

View PR using the GUI difftool:
$ git pr show -t 9176

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9176.diff

[bridgekeeper]

👋 Welcome back iklam! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@iklam The following labels will be automatically applied to this pull request:

• hotspot
• shenandoah

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 04: Full - Incremental (c2dce9be)
• 03: Full - Incremental (ab986620)
• 02: Full - Incremental (ba07ef2c)
• 01: Full - Incremental (159e9530)
• 00: Full (ebb83152)

[stefank]

Even though the name sounds generic, the Devirtualizer class was written to support the oop_iterate / OopIterateClosure devirtualization. (Maybe a hint that this should be renamed)

This patch splits the implementation of this devirtualization into two different files. I think that's unfortunate. I'd rather see that we kept that code together. Maybe move all the code into a new oopIterateClosure.inline.hpp file.

However, if you do so, you'll find that we would almost move all the code out of iterator.inline.hpp. To me that makes it unclear how much the move of the Devirtualizer contributed to the fixed compilation time.

Since this patch moves two completely separate closures, could you repurpose this PR to only move the SerializeClosure, so that we can evaluate the move of the OopIterateClosure separately?

[iklam]

Even though the name sounds generic, the Devirtualizer class was written to support the oop_iterate / OopIterateClosure devirtualization. (Maybe a hint that this should be renamed)

This patch splits the implementation of this devirtualization into two different files. I think that's unfortunate. I'd rather see that we kept that code together. Maybe move all the code into a new oopIterateClosure.inline.hpp file.

However, if you do so, you'll find that we would almost move all the code out of iterator.inline.hpp. To me that makes it unclear how much the move of the Devirtualizer contributed to the fixed compilation time.

Since this patch moves two completely separate closures, could you repurpose this PR to only move the SerializeClosure, so that we can evaluate the move of the OopIterateClosure separately?

The main problem is:

• Devirtualizer inline functions are simpler, but they are needed by many object files
• OopIterateClosure inline functions are complex (and have lots of dependencies, due to the expansion of OopOopIterateDispatch::Table::Table()), but they are needed by very few object files

Here's the output of https://github.com/iklam/tools/blob/main/headers/whoincludes.tcl:

/before/open/src/hotspot$ whoincludes iterator.inline.hpp
scanning    410 iterator.inline.hpp
   2 found    410 instanceStackChunkKlass.inline.hpp
   3 found     14 g1OopClosures.inline.hpp
   4 found      7 g1FullGCOopClosures.inline.hpp
   5 found      5 psPromotionManager.inline.hpp
   6 found      0 shenandoahBarrierSetClone.inline.hpp
   7 found      0 shenandoahMark.inline.hpp

=============================

/after/open/src/hotspot$ whoincludes iterator.inline.hpp
scanning     43 iterator.inline.hpp
   2 found     14 g1OopClosures.inline.hpp
   3 found      7 g1FullGCOopClosures.inline.hpp
   4 found      5 psPromotionManager.inline.hpp
   5 found      0 shenandoahClosures.inline.hpp
   6 found      0 shenandoahBarrierSetClone.inline.hpp
   7 found      0 shenandoahMark.inline.hpp

/after/open/src/hotspot$ whoincludes devirtualizer.inline.hpp
scanning    412 devirtualizer.inline.hpp
   2 found    412 instanceKlass.inline.hpp
   3 found    410 stackChunkFrameStream.inline.hpp
   4 found    410 instanceStackChunkKlass.inline.hpp
   5 found     44 instanceRefKlass.inline.hpp
   6 found     43 instanceClassLoaderKlass.inline.hpp
   7 found     43 instanceMirrorKlass.inline.hpp
   8 found     43 objArrayKlass.inline.hpp
   9 found     10 defNewGeneration.inline.hpp
  10 found      0 shenandoahMark.inline.hpp
/after/open/src/hotspot$ whoincludes stackChunkFrameStream.inline.hpp
scanning    410 stackChunkFrameStream.inline.hpp
   2 found    410 instanceStackChunkKlass.inline.hpp
/after/open/src/hotspot$ whoincludes instanceStackChunkKlass.inline.hpp
scanning    410 instanceStackChunkKlass.inline.hpp
   2 found    410 stackChunkFrameStream.inline.hpp
   3 found    410 stackChunkOop.inline.hpp
   4 found     43 iterator.inline.hpp
/after/open/src/hotspot$ whoincludes stackChunkOop.inline.hpp
scanning    410 stackChunkOop.inline.hpp
   2 found    410 stackChunkFrameStream.inline.hpp
   3 found    410 frame.inline.hpp
   4 found    410 instanceStackChunkKlass.inline.hpp
   5 found    410 javaClasses.inline.hpp
   6 found     35 continuationGCSupport.inline.hpp
   [...]

So the main benefit of this PR is that the expensive expansion of OopIterateDispatch is no longer needed by the popular header stackChunkFrameStream.inline.hpp (introduced by Loom, and is indirectly used by the popular header frame.inline.hpp, which I looked at but is much more difficult to refactor).

So keeping OopIterateDispatch and Devirtualizer in the same file doesn't make sense, as long as compilation time is concerned. Maybe we can add a comment to describe their relationship?

Also, as shown in the output above, apparently the Loom project found a use case of Devirtualizer without using OopIterateDispatch at the same time (see StackChunkFrameStream<frame_kind>::iterate_derived_pointers()).

For completeness and clarity, I can do three separate PRs:

• limit this PR to Devirtualizer only
• move OopIterateDispatch in PR no. 2
• move SerializeClosure in PR no. 3

And devirtualizer.hpp/oopIterateDispatch should be moved to the share/oops directory?

What do you think?

[stefank]

I forgot that Loom hijacked Devirtualizer for the derived oops processing. This devirtualization is not needed. The compiler sees the correct type and performs the correct inlining anyway. Before the integration of Loom I removed the usage of Devirtualizer::do_bit and verified that the compiler generated the same code (though I forgot to remove function).

I took and tested removing Devirtualizer::do_derived_oops, and again, the compiler generated the same code. So, simplest would be to just remove that function and the include of iterator.inline.hpp. However, if you still want to do this separation, then I guess that's OK as well. Could you move the Devirtualizer class to utilities/ instead of oops/, given that it's not necessarily oops that we are visiting? Maybe also update the comment to not mention OopClosureType.

Sounds good to move the OopIterateDispatch code to oops/oopIterateDispatch.

[iklam]

I forgot that Loom hijacked Devirtualizer for the derived oops processing. This devirtualization is not needed. The compiler sees the correct type and performs the correct inlining anyway. Before the integration of Loom I removed the usage of Devirtualizer::do_bit and verified that the compiler generated the same code (though I forgot to remove function).

I took and tested removing Devirtualizer::do_derived_oops, and again, the compiler generated the same code. So, simplest would be to just remove that function and the include of iterator.inline.hpp. However, if you still want to do this separation, then I guess that's OK as well. Could you move the Devirtualizer class to utilities/ instead of oops/, given that it's not necessarily oops that we are visiting? Maybe also update the comment to not mention OopClosureType.

Sounds good to move the OopIterateDispatch code to oops/oopIterateDispatch.

I think it's still useful to move Devirtualizer out in case it might be used in the future by other code. I've backed out the changes related to SerializeClosure, and moved the header to utilities/devirtualizer.hpp.

If you want to do the Loom cleanup, I'll leave it up to you :-)

[stefank]

Sounds good to me :)

[stefank]

Since these new files are mainly copied code, would you mind retaining the copyright dates. Right now this look like newly created code.

[iklam]

I updated the copyright year to 2018-2022, since the Devirtualizer class was first added in 2018 in JDK-8204540

[coleenp]

Looks like an improvement.

[openjdk]

@iklam This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8288537: Move Devirtualizer out of hotspot/share/memory/iterator.hpp

Reviewed-by: stefank, coleenp

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[iklam]

Thanks @stefank @coleenp for the review.
Passes tier-1 and builds-tier5 in our CI. GHA seems to work, too.

/integrate

[openjdk]

Going to push as commit 9f8bfab.
Since your change was applied there has been 1 commit pushed to the master branch:

• f080430: 8288599: com/sun/management/OperatingSystemMXBean/TestTotalSwap.java: Expected total swap size ... but getTotalSwapSpaceSize returned ...

Your commit was automatically rebased without conflicts.

[openjdk]

@iklam Pushed as commit 9f8bfab.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.