search

googlefonts / fontc

Where in we pursue oxidizing (context: https://github.com/googlefonts/oxidize) fontmake.
Apache License 2.0
73 stars 12 forks source link

Investigate performance regression #369

Closed cmyr closed 1 year ago

cmyr commented 1 year ago 
          > is almost 4 times slower than when I also revert https://github.com/googlefonts/fontc/pull/359

maybe this warrants its own issue. Something is going on with #359 that has slowed down things noticeably. Building GoogleSans.designspace takes 27 seconds after #359, and only 5 seconds if I revert it (timings are with fontc built in release mode and with --emit-ir=false, debug or emit-ir=true make it even slower). I am not able to figure out what in #359 would produce such a major (5x) slow-down.

Originally posted by @anthrotype in https://github.com/googlefonts/fontc/issues/365#issuecomment-1632981997

rsheeter commented 1 year ago

+1 to worth it's own issue.

You can assign to me, slowdown from 359 is expected, I didn't have time to tune prior to OOO and wanted to land as I'm quite confident in the general direction of the refactoring. I also have some ideas about improving visibility into how the work graph executed.

I would anticipate delay from 1) deliberately naive implementation (beware of premature optimization and all that) and 2) changes to how glyph processing threads: be glyphs current block in all fe glyphs, not just the ones they depend on. Both issues should be fixable w/o inordinate issue; the actual work to be done hasn't changed.

On Wed, Jul 12, 2023, 3:38 PM Colin Rofls @.***> wrote:

      > is almost 4 times slower than when I also revert https://github.com/googlefonts/fontc/pull/359

maybe this warrants its own issue. Something is going on with #359 https://github.com/googlefonts/fontc/pull/359 that has slowed down things noticeably. Building GoogleSans.designspace takes 27 seconds after

359 https://github.com/googlefonts/fontc/pull/359, and only 5 seconds

if I revert it (timings are with fontc built in release mode and with --emit-ir=false, debug or emit-ir=true make it even slower). I am not able to figure out what in #359 https://github.com/googlefonts/fontc/pull/359 would produce such a major (5x) slow-down.

Originally posted by @anthrotype https://github.com/anthrotype in #365 (comment) https://github.com/googlefonts/fontc/issues/365#issuecomment-1632981997

— Reply to this email directly, view it on GitHub https://github.com/googlefonts/fontc/issues/369, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRKXAHEY3AJ4QB7XOMJUWTXP3VLJANCNFSM6AAAAAA2H4GAOU . You are receiving this because you are subscribed to this thread.Message ID: @.***>

rsheeter commented 1 year ago

https://github.com/bheisler/iai might be worth a look; if it works we could have CI for perf for a small set of exemplar fonts.

cmyr commented 1 year ago

agree that this looks promising. I wonder if someone else has done the work to integrate that into github actions...

rsheeter commented 1 year ago

After #380 if I flamegraph Oswald (cmd below) the dominant factors are Font::load (16%) and and iup_delta_optimize (22%). It also looks like perhaps we would benefit from some buffer preallocation in glyph ir, will file a separate issue for that.

As none of this is new due to #359 I'd be inclined to call this fixed if the results hold up for Google Sans.

rm -rf build/ && cargo flamegraph -p fontc --root --  --source ../OswaldFont/sources/Oswald.glyphs --emit-ir false