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.

Sign up for GitHub

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

Jump to bottom

Performance regression in 4.41.0? #3213

Closed
imran-iq opened this issue Jul 19, 2023 · 4 comments · Fixed by #3214
Closed

Performance regression in 4.41.0? #3213

imran-iq opened this issue Jul 19, 2023 · 4 comments · Fixed by #3214

Comments

@imran-iq
Copy link

See: satbyy/go-noto-universal#59

Using 4.41.0 the builds take longer than 6 hours, but using 4.40.0 it takes 20ish minutes. Not too sure what is going on since I am not familiar with either project but just thought I would point it out
@anthrotype
Copy link
Member

Thanks for the report. However I don't immediately see anything related to pyftsubset in the changes since 4.40.0 that might have caused such regression:

4.40.0...4.41.0

I'll keep investigating

anthrotype added a commit that referenced this issue Jul 20, 2023
@anthrotype
[subset] Revert use of NameVisitor in subsetter which may be causing …
f0d601a 
…speed regression

Intended to fix #3213

Revert "Implement Cosimo feedback"

This reverts commit cfede76.

Revert "remove redundant CPAL name removal code"

This reverts commit b563941.

Revert "convert name table _prune_pre_subset to prune_post_subset"

This reverts commit 0a6e8bf.

Revert "Use NameRecordVisitor in subsetter"

This reverts commit edf8891.
@anthrotype
Copy link
Member

I suspect it might have to do with this PR where we use a new NameVisitor object to traverse the entire font to look for any nameID that may be used in various tables: #3185
For huge fonts like the ones in go-noto-universal repository, that might make a big difference.
I'll try to revert the changes from that PR which are related to the subsetter, maybe @imran-iq you could try building using the patched fonttools and see if that fixes the regression?

@anthrotype anthrotype mentioned this issue Jul 20, 2023
Merged
@imran-iq
Copy link
Author

Just posting here for record keeping, but as mentioned in the pr: #3214 (comment)

I have run the script with the patched fonttools and no longer see the issue (pyftmerge does not take 6 hours)

@behdad
Copy link
Member

behdad commented Jul 20, 2023

Perhaps we can call the visitor only on certain tables. I'm guessing that it's expanding all glyf's here causing the slowdown.

@behdad behdad mentioned this issue Jul 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[subset] Revert use of NameVisitor to fix speed regression fonttools/fonttools
3 participants
@behdad @imran-iq @anthrotype