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

Delete code pertaining to incremental compilation #1183

Merged
merged 1 commit into from
Jan 4, 2025
Merged

Delete code pertaining to incremental compilation #1183

merged 1 commit into from
Jan 4, 2025

Conversation

rsheeter
Copy link
Contributor

@rsheeter rsheeter commented Dec 9, 2024

Rename --incremental to --emit-ir; delete all code pertaining to incremental operation, simplify the IR Source trait as a result. Net reduction of ~2k loc, simpler execution path, and less work done for a clean compile.

For context, the incremental code was written prior to understanding just how dynamic the graph of tasks can be. We default it to off and it's not clear its completely reliable. If we find we want incremental one day we can start fresh; until then it's much simpler to just not do all the associated bookkeeping.

Apologies for the huge diff, it's almost all just deleting bookkeeping.

My intuition is this will lead to further simplifications.

We're doing less work so it shoudl be faster, but is it? - timed on M1 MacBook it appears it is:

# fontmake
$ (cd ~/oss/googlesans && rm -rf variable_ttf && time fontmake -o variable -- source/GoogleSans/GoogleSans.designspace)
real	3m4.600s

# fontc, main vs nocs branch
$ hyperfine \
  --parameter-list branch main,nocs \
  --setup "git checkout {branch} && cargo build --release" \
   --warmup 5 --runs 20 \
   --prepare 'rm -rf build/' \
   "target/release/fontc --source ../googlesans/source/GoogleSans/GoogleSans.designspace"

Benchmark 1: target/release/fontc --source ../googlesans/source/GoogleSans/GoogleSans.designspace (branch = main)
  Time (mean ± σ):      4.137 s ±  0.032 s    [User: 7.495 s, System: 7.130 s]
  Range (min … max):    4.068 s …  4.180 s    20 runs
 
Benchmark 2: target/release/fontc --source ../googlesans/source/GoogleSans/GoogleSans.designspace (branch = nocs)
  Time (mean ± σ):      3.465 s ±  0.041 s    [User: 7.351 s, System: 6.561 s]
  Range (min … max):    3.348 s …  3.529 s    20 runs
 
# THE IMPORTANT PART
Summary
  target/release/fontc --source ../googlesans/source/GoogleSans/GoogleSans.designspace (branch = nocs) ran
    1.19 ± 0.02 times faster than target/release/fontc --source ../googlesans/source/GoogleSans/GoogleSans.designspace (branch = main)

# That is, on main we build GS ~45x faster than fontmake and on this branch we build it ~53x faster.

JMM if happy.

@cmyr
Copy link
Member

cmyr commented Dec 9, 2024

Assuming no performance impact I think this will be a huge win, and I share the intuition that getting rid of it will unlock a range of other improvements.

@rsheeter
Copy link
Contributor Author

rsheeter commented Dec 9, 2024

Oh I maybe worded that poorly, if there is a perf impact I expect it to be positive.

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.

just did a skim, looks very promising!

fontc/src/change_detector.rs Show resolved Hide resolved
fontc/src/lib.rs Show resolved Hide resolved
fontc/src/lib.rs Outdated Show resolved Hide resolved
fontir/src/error.rs Outdated Show resolved Hide resolved
fontir/src/serde.rs Outdated Show resolved Hide resolved
fontir/src/source.rs Outdated Show resolved Hide resolved
@rsheeter rsheeter force-pushed the nocs branch 6 times, most recently from b496e43 to bd50460 Compare December 12, 2024 21:56
@rsheeter rsheeter changed the title [WIP] Delete code pertaining to incremental compilation Delete code pertaining to incremental compilation Dec 12, 2024
@rsheeter rsheeter marked this pull request as ready for review December 12, 2024 21:56
@rsheeter rsheeter force-pushed the nocs branch 2 times, most recently from 66eb2fb to b607cee Compare January 3, 2025 12:50
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.

looks good, very happy to see this merged!

@rsheeter rsheeter added this pull request to the merge queue Jan 4, 2025
Merged via the queue into main with commit 45efcf7 Jan 4, 2025
10 checks passed
@rsheeter rsheeter deleted the nocs branch January 4, 2025 18:05
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.

2 participants