Skip to content
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

Add amalgamation source file #1809

Closed
behdad opened this issue Jul 2, 2019 · 10 comments
Closed

Add amalgamation source file #1809

behdad opened this issue Jul 2, 2019 · 10 comments

Comments

@behdad
Copy link
Member

behdad commented Jul 2, 2019

Add hb.cc that will include all base .cc files (easy) then make it actually compile (more work).

behdad added a commit that referenced this issue Jul 2, 2019
behdad added a commit that referenced this issue Jul 2, 2019
behdad added a commit that referenced this issue Jul 2, 2019
behdad added a commit that referenced this issue Jul 2, 2019
behdad added a commit that referenced this issue Jul 2, 2019
behdad added a commit that referenced this issue Jul 2, 2019
This actually makes it build now!

Part #1809

Keeping open to add tests, CI, etc.
@behdad
Copy link
Member Author

behdad commented Jul 2, 2019

This works now!

Compile time is ~10s to 20s. I also see some measurable size improvement... Not jumping to conclusions just yet though.

Leaving open to 1. document in CONFIG.md, and 2. add bots that use it.

Note the resulting hb.cc doesn't add hb-ft.cc, hb-glib.cc, .. the integrations need to be added separately.

behdad added a commit that referenced this issue Jul 2, 2019
@behdad
Copy link
Member Author

behdad commented Jul 2, 2019

@drott @jfkthame FYI. I know Firefox already tries this itself. Might want to switch to this (when released).

@behdad
Copy link
Member Author

behdad commented Jul 2, 2019

cc @nona-google as well.

@ebraminio
Copy link
Collaborator

Niiiice

@drott
Copy link
Collaborator

drott commented Jul 3, 2019

Thanks, yes, in Chrome we call these "jumbo builds", and I think we have some tools in our .gn build files for this approach. But it's good if we can simplify our HarfBuzz build file. Are you testing both configurations on the CI bot, building the amalgamated and the regular files?

@behdad
Copy link
Member Author

behdad commented Jul 3, 2019

Are you testing both configurations on the CI bot, building the amalgamated and the regular files?

Not yet, but that's the plan indeed.

behdad added a commit that referenced this issue Jul 3, 2019
behdad added a commit that referenced this issue Jul 3, 2019
Just for those that are normally built into libharfbuzz itself.

Part of #1809
@ebraminio
Copy link
Collaborator

We can always use harfbuzz.cc in cmake which has the test suite, that way we don't have to modify autotools development and bots and make this test covered, deferring more to after meson migration.

@ebraminio
Copy link
Collaborator

ebraminio commented Jul 5, 2019

Are you testing both configurations on the CI bot, building the amalgamated and the regular files?

As 9fea6b4 we now have harfbuzz.cc both build and test-suite checked in different cmake CI bots (including two Windows bots and bots testing all the shaping results tests), you will get some build/link time performance and size benefits also in Chrome (as it happened in harfbuzzjs) as the result I'd guess but harfbuzz.cc doesn't include subset files so you should include them separately, hmmm, maybe we should add those also when some compile flag is set, for now would be nice to test them in Chrome build but don't merge the change till the release so Behdad won't need to rush on the details.

@ebraminio
Copy link
Collaborator

harfbuzz.cc is released so let's close this, yay!

@ebraminio
Copy link
Collaborator

ebraminio commented Aug 18, 2019

Filed https://crbug.com/995090 @drott this is being built with our cmake which is test covered so no worry about correctness AFAICT.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants