-
Notifications
You must be signed in to change notification settings - Fork 5.8k
8348830: LIBFONTMANAGER optimization is always HIGHEST #23332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into |
@MBaesken 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:
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 11 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Hi Erik, thanks for the review ! I noticed this issue, while going through the make rules of our libs; any idea why so many are set to OPTIMIZATION level LOW ? Is this really still needed or just because of old compilers used many years ago? Lib compilation current optimization level |
Not all libraries do especially performance critical things. |
Thanks for the reviews !
Agree, it was not my suggestion to set everywhere HIGH/HIGHEST . The question I had is - are there libraries where we set LOW because higher/other opt-levels would break the library (compilation and also runtime - wise) ? |
/integrate |
Going to push as commit 168a471.
Your commit was automatically rebased without conflicts. |
Regarding BUILD_LIBAWT_XAWT I wonder why LOW is set; there is some java2d / OGL coding in the lib and there the performance is probably important? java.desktop/libawt_xawt/AccelGlyphCache.o |
The problem with changing optimization levels is that folks are generally conservative and worried that things might break, that are hard to detect. I'm not convinced that the risk of changing optimization level is any larger than upgrading the compiler though, so maybe that is not a very good argument. As has been pointed out e.g. by Phil, not all libraries are performance critical. With that said, maybe such libraries should be optimized for size instead? (In many cases that also brings along a speed performance, even if it by no means guaranteed.) Changing optimization levels should not only be measured on the execution speed of the final product, but also on how much it affects the build time. If you feel up for it, I suggest that you go through all libraries you want to update, change the optimization levels to what you think is reasonable, measure any relevant performance benchmarks before and after, and also the the build time. (hint: Use e.g. Then you can post a PR and suggest that we change the optimization level. I'm all in favor of this, I'm not just in favor of having to do all the work needed myself. :-) |
Hi Magnus, that sounds like a good idea, especially for libraries that are a bit larger (if they are below 100K it does probably not make a big difference).
BUILD_LIBJSVML is also rather large (~ 850K) but has no opt-level set for the lib make rule; not sure what that means, almost all other libs set a specific optimization level . ( Besides changing a couple of default opt-levels for some libs, I also would like to have better configure functionality to set it myself like we have for libjvm with the 'jvm-features' . ) |
I don't think there is much point in being able to vary the opt level; it is more about spending the time to find the most appropriate opt level, and once you've done that, you can update the makefile directly. |
You want to optimize for binary size in some scenarios but not in all. Still, for some simple cases like e.g. BUILD_LIBSPLASHSCREEN changing the opt level from LOW to SIZE is probably the best thing to do after some more testing. |
I think LTO ability for native JDK code could be worthwhile as a flag, but having a JVM opt-size equivalent for the JDK native code is not as beneficial, given not many are really interested in the individual optimization levels of each module. In any case LibCommon.gmk and LauncherCommon.gmk is where I put these optimization overrides in my fork |
The size optimization worked pretty well on most platforms , have to look still more into the compilation times of the this lib for a complete picture. No jtreg/jck failures seen. However for MSVC the SIZE optimization flags seem to be bad , the binaries get LARGER not smaller ! I opened https://bugs.openjdk.org/browse/JDK-8349214 . |
That does not work/compile, it leads to
Seems the dependencies do not work for some reason, so I need to build the whole jdk with LOG=info I guess. |
Adding the
Then delete only the things you want to rebuild and run the part of the build that is relevant. This can be repeated.
|
In the makefile we reset LIBFONTMANAGER optimization, but is always set to HIGHEST so we can avoid the resetting.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23332/head:pull/23332
$ git checkout pull/23332
Update a local copy of the PR:
$ git checkout pull/23332
$ git pull https://git.openjdk.org/jdk.git pull/23332/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23332
View PR using the GUI difftool:
$ git pr show -t 23332
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23332.diff
Using Webrev
Link to Webrev Comment