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

Spread BE kerning across threads #644

Merged
merged 2 commits into from
Dec 13, 2023
Merged

Spread BE kerning across threads #644

merged 2 commits into from
Dec 13, 2023

Conversation

rsheeter
Copy link
Contributor

@rsheeter rsheeter commented Dec 12, 2023

Builds on #642.

Numbers bounce around a bit run to run but mean + σ on branch is consistantly better than mean - σ on main.

$ cargo build --release && hyperfine --warmup 5 --prepare 'rm -rf build/' 'target/release/fontc ../googlesans/source/GoogleSans/GoogleSans.designspace'

# Linux
main   6.925 s ±  0.248 s
branch 6.356 s ±  0.217 s

# Mac
main  3.904 s ±  0.370 s
branch 3.828 s ±  0.037 s

Linux $ cargo build --release && rm -rf build/ && target/release/fontc ../googlesans/source/GoogleSans/GoogleSans.designspace --emit-timing:

image

Observations:

  • fea is 15%
  • font, which per Generating GPOS takes a long time #610 is mostly packing gpos, is 20%
  • kernat should probably start sooner, such as right after glyph order
    • I need to take another pass at making sure we refer to kern instance not kern at :)

@rsheeter rsheeter changed the title [WIP] Try spreading BE kerning across threads Spread BE kerning across threads Dec 12, 2023
@rsheeter rsheeter marked this pull request as ready for review December 12, 2023 23:14
fontbe/src/orchestration.rs Outdated Show resolved Hide resolved
fontbe/src/paths.rs Outdated Show resolved Hide resolved
fontbe/src/paths.rs Outdated Show resolved Hide resolved
fontbe/src/orchestration.rs Outdated Show resolved Hide resolved
fontbe/src/orchestration.rs Outdated Show resolved Hide resolved
fontbe/src/kern.rs Outdated Show resolved Hide resolved
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the main thing missing from all of this, as best I can tell, is some high-level overview of how it works: basically what are the different stages of the work and how to they fit together, what are the invariants at each stage, etc; I think that would be very useful to have somewhere.

orchestration::WorkId as FeWorkId,
};
use ordered_float::OrderedFloat;
use log::debug;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as a general observation but I prefer not to import these macros by name, and instead just call them as log::debug!("my message"). The main rationale for this is simply that it makes it easier to grep for all the places where you're logging. A secondary rationale is that there's a (loose) convention in Rust to avoid importing functions directly, and instead import their parent module.. I can't find a great source for this, but it's mentioned here by klabnik. I like this convention but am not zealous about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do this relatively consistently; maybe file an issue and we'll fix it eveyrwhere plus add a presubmit?

fontbe/src/kern.rs Outdated Show resolved Hide resolved
fontbe/src/kern.rs Outdated Show resolved Hide resolved
fontbe/src/orchestration.rs Outdated Show resolved Hide resolved
@rsheeter rsheeter added this pull request to the merge queue Dec 13, 2023
Merged via the queue into main with commit fe69d63 Dec 13, 2023
10 checks passed
@rsheeter rsheeter deleted the kp2 branch December 13, 2023 21:26
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

Successfully merging this pull request may close these issues.

3 participants