Skip to content

[read-fonts] inline all the things#1584

Open
dfrg wants to merge 1 commit into
mainfrom
inline-lots
Open

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

Conversation

@dfrg

@dfrg dfrg commented Aug 5, 2025

Copy link
Copy Markdown
Contributor

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

@behdad

behdad commented Aug 5, 2025

Copy link
Copy Markdown
Contributor

Haha. Nice one. Will measure soon.

@rsheeter rsheeter left a comment

Copy link
Copy Markdown
Contributor

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

behdad commented Aug 5, 2025

Copy link
Copy Markdown
Contributor

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

behdad commented Aug 5, 2025

Copy link
Copy Markdown
Contributor

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