8365053: Refresh hotspot precompiled.hpp with headers based on current frequency #26681
Conversation
|
👋 Welcome back fandreuz! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@fandreuz To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command. Applicable Labels
|
|
/label hotspot build |
|
@fandreuz The |
Webrevs
|
|
Also, maybe check in the generation script as well? I think a subfolder here would be fine: https://github.com/openjdk/jdk/tree/master/src/utils. It would be nice if the output of that script could be simply piped into We can, technically, hook it up a similar way SortIncludes.java is currently done, but I think it is unnecessary at this point. In a perfect world we would not be needing precompiled headers. In less ideal world, having a jtreg test that warns us that a new popular header appeared, or that older header is not as popular anymore, would be handy. Again, this is something out of scope for this PR. |
|
On my M1, with some AV software that always get in the way, I am seeing 15% faster build, which saves about half a minute. This is great! |
|
I tried it on Windows. Build time for hotspot was basically the same before and after, so no regression at least. So I'm fine with this change. |
I'll add the generation script, it looks like something that could be useful at some point in the future. I think there should be a human curator though and not respecting the check should not count as e.g. a test failure, because the process is not completely automatic. For example, when the threshold is set to 50 I get this compilation error: It could happen with any threshold, even 130 as the codebase evolves. |
How can we possibly get a compilation error when everything should build with PCH disabled? |
|
|
||
| // Count inclusion times for each header | ||
| Map<String, Integer> occurrences = new HashMap<>(); | ||
| try (Stream<Path> paths = Files.walk(hotspotPath)) { |
There was a problem hiding this comment.
I think walking the source tree is the wrong approach to gathering the data
about include counts. I think better is to do a build and then look at the
files /hotspot/variant-server/libjvm/objs/*.d. From that one can build
a completely accurate count of the transitive inclusions.
There was a problem hiding this comment.
I followed this hint, the latest version of the script checks the .d files.
Two observations:
- The magic number is now
2460(more includes are taken into account, which makes sense) - There is much less wiggle room, 2461 includes nothing, 2459 includes too much
- Runtime does not seem to be affected negatively or positively
If anybody is interested, this file contains the inclusion count for each source file: inclusions_count.txt
There was a problem hiding this comment.
Something seems wrong with these numbers.
- There are 538 headers with include counts of 2460.
- There are only 1121 .o.cmdline files.
So how do we get include counts that are greater (by more than a factor of 2)
than the number of .d files?
Note that precompiled.hpp has an include count of 2456. There's 1 .d file that
depends on everything - BUILD_LIBJVM.d. That probably ought to be excluded
from counting, although I guess it should just add one to every file.
Also, precompiled.hpp has an include count of 2456, so not the maximum count,
but close. Which is itself weird that it wouldn't be the most included file.
But it's hard to guess what might be going on with that until the surprising
high include count > number of .d files is understood.
That include count listing would also be more useful of sorted by count.
There was a problem hiding this comment.
Note that precompiled.hpp has an include count of 2456. There's 1 .d file that
depends on everything - BUILD_LIBJVM.d. That probably ought to be excluded
from counting, although I guess it should just add one to every file.
Apparently BUILD_LIBJVM.d mentions all files multiple times:
% grep "utilities/waitBarrier.hpp" build/clang/hotspot/variant-server/libjvm/objs/BUILD_LIBJVM.d | wc -l
1231
It's definitely reasonable to exclude it, I did it in the latest revision of the script.
Now, the numbers seem more reasonable:
% ls build/clang/hotspot/variant-server/libjvm/objs/*.d | grep -v BUILD | wc -l
1232
% grep "code/codeCache.hpp" build/clang/hotspot/variant-server/libjvm/objs/*.d | \
grep -v BUILD | sort | uniq | wc -l
1230
% grep "code/codeCache.hpp" inclusions_count.txt
code/codeCache.hpp=1230
Updated inclusions_count.txt
There was a problem hiding this comment.
I assume this means that other .d files might include a header multiple times, too. Is it reasonable to count only the number of files containing a header instead of number of appearances?
There was a problem hiding this comment.
I assume this means that other .d files might include a header multiple times, too.
From the following two numbers, I deduce code/codeCache.hpp is (transitively) included in 1230 files, with each inclusion coming from a unique .d file. Am I missing something?
% grep "code/codeCache.hpp" build/clang/hotspot/variant-server/libjvm/objs/*.d | grep -v BUILD | sort | wc -l
1230
% grep "code/codeCache.hpp" build/clang/hotspot/variant-server/libjvm/objs/*.d | grep -v BUILD | sort | uniq | wc -l
1230
I haven't checked every single file, but I'd expect each include to appear at most once in each .d file.
There was a problem hiding this comment.
Thanks for the clarification, I think that it is fine, then, although a comment explaining in the code would be great.
There was a problem hiding this comment.
I suspect that the multiple includes reported in the .d files are accurate. The .d file appears
to report the number of times we #include a file, however, because if the include-guard
constructs we use, the content is pulled in only once.
There was a problem hiding this comment.
The number of times a header is (directly or indirectly) included by a given
source file doesn't affect the number of times that header appears in the .d
file associated with that source file. The compiler uniquifies that set
(though links might confuse things, but that's not an issue here).
The .d files consist of a set of targets, and for each target, the set of
dependencies.
For the .d files, there's one target, .o, with its
dependencies (once each).
For BUILD_LIBJVM.d, there's a target for each .o file. And for each of those
targets, it's dependencies (once each). Essentially BUILD_LIBJVM.d includes a
concatenation of all the .d files. And since there are duplicates
among the dependencies of different targets, there are many(!) duplicates in
this file.
| @@ -26,52 +26,38 @@ | |||
| // --disable-precompiled-headers to configure. | |||
|
|
|||
| // These header files are included in at least 130 C++ files, as of | |||
| // measurements made in November 2018. This list excludes files named | |||
| // *.inline.hpp, since including them decreased build performance. | |||
| // measurements made in August 2025. | |||
There was a problem hiding this comment.
I don't think there's anything particularly special about the number 130.
Another thing to consider when a header file has a high include count is
whether it's being overincluded. We've had lots of those, and some folks
occasionally try to poke at that problem. Some of the removals here look like
they might be a result of such efforts.
Still another thing to consider is the cost of inclusion. Some files may just
be a lot more expensive to process and benefit more for being precompiled.
File size can be an indicator, but there are others. Unfortunately, I don't
know of a good way to measure this.
There was a problem hiding this comment.
You need to modify the comment here, too.
There was a problem hiding this comment.
Right, I'll wait just a bit so the discussion on how to approach the problem stabilizes :)
There was a problem hiding this comment.
This number is based on my original experimentation. If I tried to lower the bar by lowering the number, the PCH list grew too much and it made for a worse performance. And contrary, if I raised the number, fewer files where included which made the PCH quicker to process but less helpful. That number was the optimum I found. It seems from https://bugs.openjdk.org/browse/JDK-8365053 that this is still around the optimum. However, rather than just mentioning the number in the comment, the rationale could be specified, like the optimum number of includes.
| #include "utilities/globalDefinitions.hpp" | ||
| #include "utilities/growableArray.hpp" | ||
| #include "utilities/macros.hpp" | ||
| #include "utilities/ostream.hpp" | ||
| #include "utilities/ticks.hpp" | ||
|
|
||
| #ifdef TARGET_COMPILER_visCPP |
There was a problem hiding this comment.
All of the reported testing was on Linux. These were included specifically because measurements
said their inclusion here was beneficial. And my recollection from previous discussions is that
Visual Studio may be the compiler where precompiled headers are most beneficial.
There was a problem hiding this comment.
Indeed, Visual Studio is the place where PCH is most needed. I see Erik says he tested on Windows with no difference. While he concluded that this means no regression, I see it as a missed opportunity. Giving the Windows platform a bit of extra love can probably increase compilation speed where it is needed the most.
Possibly some includes are inside namespaces or classes? I would have to check on this though. |
| @@ -26,52 +26,38 @@ | |||
| // --disable-precompiled-headers to configure. | |||
|
|
|||
| // These header files are included in at least 130 C++ files, as of | |||
| // measurements made in November 2018. This list excludes files named | |||
| // *.inline.hpp, since including them decreased build performance. | |||
| // measurements made in August 2025. | |||
There was a problem hiding this comment.
You need to modify the comment here, too.
|
The latest version fails for me. I'm guessing because we exclude shenandoah by default for OracleJDK builds so having those headers in precompiled.hpp messes things up. I think we need to be careful with headers that are part of optional features. |
The last commit (be25d34) should fix the problem. I made a build with the following configuration: and then I ran the script on this build to extract the inclusions count. This could be included in the README file attached to this PR. |
Other optional features include cds, c1, jfr, and jvmci, so files from those The (current) list of optional features is here: Not all of these have their own directories. Even where they do, the feature I didn't spot any files for other optional features in the latest list, other |
Can't these just be conditionalized with an INCLUDE_xxx check instead of being excluded? |
|
There is a handy shortcut for creating a build with all optional features disabled, |
Yeah, that might be better than excluding altogether. But still need to figure out which files to (conditionally) exclude. |
@shipilev I'm not sure this is a proper place. The other tools in |
Maybe first make a baseline build with no optional features, and analyze the result with this tool. And then enable each individual feature, re-build and re-analyze, and see how it differs. If a file then makes it to the PCH list, it is useful when enabling that feature, and it is potentially only correct to include with that feature enabled, so it could be added with an include guard. That will require a bit more manual work (unless this process too can be automated), but it will probably give the best results. |
Hi @magicus, thanks for the hint. I'll give it a shot and post the results here! |
|
Some results:
So, the third column measures how much we improve by taking new precompiled headers due to a specific features. I'd say the following are worth keeping behind an include guard:
|
I thought these should not really affect the set of files compiled (or included, which is what matters here)? |
|
That's right @magicus, I rerun the script and I got only a I'd say, as a criteria we could use is:
|
|
Just found a bug in the script, I'm rerunning the whole thing. |
|
|
This looks like regressions for c1 and G1..? And/or a lot more variability in build time. Any idea what is causing that? |
|
I'm starting to think the high variability is a quirk of my environment. I ran the builds in a virtualized machine and I'm probably experiencing erratic behavior under high load. I re-ran each combination fewer times (3, should still be enough to get an idea), and I'm observing more reasonable results:
This is the script I used. |
|
Can you re-run the test with your updated and more stable testing setup with a default, out-of-the-box configured |
|
Your updated result seemed more reliable and probable, but they also do not differ that much from the baseline. I still saw a few improvements and no regression, so it might still be worth pursuing, but maybe your suggested change does not bring as much improvement as was first hinted at. |
I'd say individual improvements are a bit harder to observe in the kind of measurements I'm doing now, and the likelihood of measuring noise is higher. Also, the minimum inclusion threshold I'm using now is the same for all features, but that's unlikely to be optimal. Maybe we could revert to the original approach (count inclusion for a typical build)? That proved to improve compile time, and I could hide non-standard headers behind an |
|
If I recall correctly the original approach was even simpler: I counted the number of I think your approach seems more valid, but maybe we get stuck in details then instead... But then again, this is basically an optimization problem (even if it is not about running code in the JVM, but about optimizing build speed) so there is no way around the mantra of measure, measure and measure again. |
That was also the initial approach in this PR, I got the suggestion from the first comments I got here.
Yeah I see, I'll give it another shot next week. |
In this PR I propose to refresh the included headers in hotspot
precompiled.hpp. The current set of precompiled headers was refreshed in 2018, 7 years ago. I repeated the same operations and measurements after refreshing the set of precompiled headers according to the current usage frequency.These are the results I observed. Depending on the platform, the improvement is between 10 and 20% in terms of total work (user+sys). The results are in seconds.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26681/head:pull/26681$ git checkout pull/26681Update a local copy of the PR:
$ git checkout pull/26681$ git pull https://git.openjdk.org/jdk.git pull/26681/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26681View PR using the GUI difftool:
$ git pr show -t 26681Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26681.diff
Using Webrev
Link to Webrev Comment