Skip to content

[read-fonts] inline all the things#1584

Open
dfrg wants to merge 1 commit intomainfrom
inline-lots
Open

[read-fonts] inline all the things#1584
dfrg wants to merge 1 commit intomainfrom
inline-lots

Conversation

@dfrg
Copy link
Member

@dfrg dfrg commented Aug 5, 2025

Just adds a bunch of #[inline] attributes to codegen'd functions.

@behdad
Copy link
Contributor

behdad commented Aug 5, 2025

Haha. Nice one. Will measure soon.

Copy link
Collaborator

@rsheeter rsheeter left a comment

Choose a reason for hiding this comment

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

LGTM assuming it helps. Would be nice to cite metrics for improvement

@behdad
Copy link
Contributor

behdad commented Aug 5, 2025

On Mac I don't see any change whatsoever. On Linux, I actually see 5% slowdown on the ligature-heavy BM_Shape/SourceSerifVariable-Roman.ttf/react-dom.txt/harfrust benchmark and 1% slowdown in other benches.

I'm okay merging, but looks like our config https://github.com/harfbuzz/harfbuzz/blob/main/src/rust/Cargo.toml already does a good job of inlining.

Perhaps the config should be documented in the HR repo itself, or enabled there by default.

@behdad
Copy link
Contributor

behdad commented Aug 5, 2025

On Linux, I actually see 5% slowdown on the ligature-heavy BM_Shape/SourceSerifVariable-Roman.ttf/react-dom.txt/harfrust benchmark and 1% slowdown in other benches.

I rebuilt with release mode, which means codegen-units 1 instead of 16, and I see no perf diff whatsoever. So, LGTM.

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