-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
8319197: Exclude hb-subset and hb-style from compilation #16440
Conversation
👋 Welcome back djelinski! A progress list of the required criteria for merging this PR into |
@djelinski The following labels will be automatically applied to this pull request:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me from a build point of view, but needs someone from client to approve as well.
/reviewers 2
@djelinski 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 60 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
You may want to wait for Phil's approval though.
# hb-subset and hb-style APIs are not needed, excluded to cut on compilation time. | ||
LIBFONTMANAGER_EXCLUDE_FILES += hb-ft.cc hb-subset-cff-common.cc \ | ||
hb-subset-cff1.cc hb-subset-cff2.cc hb-subset-input.cc hb-subset-plan.cc \ | ||
hb-subset.cc hb-subset-instancer-solver.cc gsubgpos-context.cc hb-style.cc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm quite confused from where you got gsubgpos-context.cc. I don't see any file of that name.
Also I just realised that since the Nov 2020 import of freetype it has a bunch of handly ifdefs that
you can see in hb-config.h
And so probably a better way of doing this, even if it means one of these files (the instance-solver one) doesn't seem to get the ifdef treatment is to use the specific defines passed to the build.
Then I'd expect harfbuzz to apply the same ifdef even if these files got refactored.
I further noticed that the same 2020 update adds HAVE_FREETYPE and hb-ft.cc isn't compiled unless you actually set that. Which we don't
So this whole exclude can go away and be replaced by a few -D defines.
You could probably go overboard and experiment with all sorts of combinations from hb-config.h but I recommend you just apply the minimal set that makes your build time happier.
hb-style.cc isn't one I'd have realised we don't currently need .. and for many of them it isn't clear to me without research what the consequences would be of excluding it. Without that support would something non-optimal happen in some cases - you can't trust the tests we have to come near proving it.
I know we don't subset, and that's the only one I consider "safe" to use here without actual research.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two
#define HB_NO_SUBSET_LAYOUT
#define HB_NO_SUBSET_CFF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can find the file here:
https://github.com/openjdk/jdk/blob/f262f06c97b9ea94cd6119b3a8beb16bf804d083/src/java.desktop/share/native/libharfbuzz/graph/gsubgpos-context.cc
Hb-subset files are not guarded by ifdefs; HB_NO_SUBSET_LAYOUT only excludes some functions, but not others. I'll check the impact of that define when I'm back in office.
I checked the list of HB APIs here ; hb-subset and hb-style belong to non-core API sets, so I decided to check if hb-style was needed. It was not.
We can safely stop compiling the files listed because:
- none of the harfbuzz functions are exported, and they are not accessible outside of libfontmanager.
- if we excluded any file that was actually used, linker would refuse to link libfontmanager
Given that we only copy an arbitrary subset of harfbuzz files anyway (see UPDATING.txt), would it make sense to remove these files instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I compiled the code without this PR, but with HARFBUZZ_CFLAGS += -DHB_NO_SUBSET_LAYOUT -DHB_NO_SUBSET_CFF
instead, and checked make LOG=profile
output. Results:
- without this change, compiling
hb-subset.cc
took 56 seconds, andhb-subset-plan.cc
took 33 seconds - with this change, compiling
hb-subset.cc
took 33 seconds, andhb-subset-plan.cc
took 22 seconds
It's a nice improvement, but not compiling these files at all is still much better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@djelinski Just curious; what would the effect be to both include this change and setting the NO_* defines?
If all the references to these defines are made in the excluded files then the only reason for doing that would be some kind of information to subsequent readers of the code, but they might also be checked elsewhere, and thus give an additional speedup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HB_NO_SUBSET_CFF is indeed referenced in 2 header files, which are in turn included somewhere else.
I added the defines on top of the excludes, but this resulted in no measurable build time difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, I tried building with HB_LEAN, which is a shortcut for "no optional features"; I got a couple of new warnings, and one error (hb_font_funcs_set_glyph_v_kerning_func
is deprecated since 2.0 and not compiled with HB_LEAN; fortunately its use in hb-jdk-font.cc can be simply commented out). After resolving these problems, fontmanager compiled successfully - but again, no measurable time difference.
So if I have this right, The build exclusion does however include another couple of files, at least one of which is unrelated, not sure if you ever said |
(1) - (4) - that's correct. My tests were hardly scientific; most of the time I only deleted and recompiled libfontmanager using 8 parallel jobs; the time taken by the recompilation was in the 50-75 seconds range both with and without HB_LEAN. File build times from a sample run, taken from build-profile.log:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let's just go with the exclusion at least for now.
If it becomes a problem on a subsequent upgrade it may need to be changed.
We can also explore other options later.
Thanks! /integrate |
Going to push as commit e1cae72.
Your commit was automatically rebased without conflicts. |
@djelinski Pushed as commit e1cae72. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
hb-subset and hb-style APIs are not used and not exported by libfontmanger. We can cut the compilation time by not compiling the unused files.
The added exclusions reduce the build time by ~1 minute (~8%) on my machine. This is with harfbuzz 8, with harfbuzz 7 the improvement was ~30 seconds.
Client libs tests continue to pass.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16440/head:pull/16440
$ git checkout pull/16440
Update a local copy of the PR:
$ git checkout pull/16440
$ git pull https://git.openjdk.org/jdk.git pull/16440/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 16440
View PR using the GUI difftool:
$ git pr show -t 16440
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16440.diff
Webrev
Link to Webrev Comment