Skip to content

refactor: Split up Primer.Core module #643

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
Merged
merged 1 commit into from
Aug 18, 2022
Merged

refactor: Split up Primer.Core module #643

merged 1 commit into from
Aug 18, 2022

Conversation

georgefst
Copy link
Contributor

I see no downsides other than the short term pain of this causing conflicts with almost any other ongoing work.

Relevant module subgraph

Before

old

After

new

nix shell nixpkgs#haskellPackages.graphmod nixpkgs#graphviz -c sh -c '
  graphmod -q \
    primer/src/Primer/Builtins.hs \
    primer/src/Primer/Builtins/DSL.hs \
    primer/src/Primer/Core.hs \
    primer/src/Primer/Core/DSL.hs \
    primer/src/Primer/Core/Utils.hs \
    primer/src/Primer/Module.hs \
    primer/src/Primer/Primitives.hs \
    primer/src/Primer/Def.hs \
    primer/src/Primer/Def/Utils.hs \
    primer/src/Primer/TypeDef.hs \
    | dot -Tsvg > out.svg
  '

georgefst
georgefst commented Aug 17, 2022
View reviewed changes
primer/src/Primer/Module.hs
, (baseName tEither, TypeDefAST eitherDef)
]
, moduleDefs = mempty
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two definitions are the only two that strike me as maybe now being in an odd place. Should our hard-wired modules be defined in the same place as the concept of a module?

But I expect we'll end up revisiting these soon enough as part of our prelude / standard library work anyway.

@georgefst georgefst requested a review from a team August 17, 2022 12:53
@georgefst georgefst mentioned this pull request Aug 17, 2022
Merged
@dhess
Copy link
Member

dhess commented Aug 17, 2022

Completely unscientific build time comparison: I will compare this PR's build of primer to that of https://buildkite.com/hackworthltd/primer/builds/1721, which is fairly close and happens to build primer from scratch. (I don't see a convenient way to see what other builds were running, unfortunately, so take this with many grains of salt. The x86_64-linux results are likely to be noisiest because the same machine that orchestrates the builds also builds products, so it is always relatively busy when a build is happening.)

Build 1721:
aarch64-linux: 14 minutes 17 seconds
aarch64-darwin: 6 minutes 15 seconds
x86_64-linux: 8 minutes 35 seconds

This build (1724):
aarch64-linux: 12 minutes 4 seconds
aarch64-darwin: 5 minutes 16 seconds
x86_64-linux: 7 minutes 24 seconds

(Quick remark that has nothing to do with this particular PR: those Mac build times are on a laptop that I've repurposed as a build machine. Unreal!)

@dhess
Copy link
Member

dhess commented Aug 17, 2022 •
edited
Loading

Here is a higher-quality test: I will build this PR in a nix develop shell using cabal build primer (after a make clean), on several different archs, and then do the same with main. These machines were unloaded at the time of building.

I also added package primer: ghc-options: -j to cabal.project.local in each instance in order to take advantage of multiple cores per haskell/cabal#976. (As far as I can tell, this is the right way to do it, but let me know if I've misunderstood.)

I will update this as results roll in.

How much speedup vs main, -O0

aarch64-darwin: effectively the same
x86_64-linux: effectively the same
aarch64-linux: effectively the same

How much speedup vs main, -O2

aarch64-darwin: 16% faster
x86_64-linux: 27% faster
aarch64-linux: 24% faster

Machine specs

aarch64-darwin: Apple M1 Max, 8 performance cores & 2 efficiency, 64 GiB RAM
x86_64-linux: AMD Ryzen 5 3600 6-Core Processor, 64 GiB RAM
aarch64-linux: Ampere Altra Q80-30 80-core processor, 128 GiB RAM

This PR, -O0

aarch64-darwin: 69.35s user 7.62s system 133% cpu 57.557 total
x86_64-linux: real 0m51.684s, user 1m11.847s, sys 0m3.213s
aarch64-linux: real 2m10.191s, user 7m51.859s, sys 0m56.063s

This PR, -O2

aarch64-darwin: 478.88s user 18.97s system 155% cpu 5:20.10 total
x86_64-linux: real 7m36.934s, user 11m26.762s, sys 0m10.055s
aarch64-linux: real 13m45.442s, user 45m25.754s, sys 5m1.315s

main, -O0

aarch64-darwin: 71.99s user 7.49s system 136% cpu 58.251 total
x86_64-linux: real 0m52.705s, user 1m12.746s, sys 0m3.326s
aarch64-linux: real 2m9.652s, user 7m35.913s, sys 0m56.671s

main, -O2

aarch64-darwin: 526.50s user 19.16s system 147% cpu 6:10.10 total
x86_64-linux: real 9m37.355s, user 13m56.945s, sys 0m12.143s
aarch64-linux: real 16m58.693s, user 51m29.894s, sys 6m1.985s

@georgefst georgefst added this pull request to the merge queue Aug 17, 2022
@georgefst georgefst removed this pull request from the merge queue due to a manual request Aug 17, 2022
@brprice brprice mentioned this pull request Aug 17, 2022
Open
@dhess
Copy link
Member

dhess commented Aug 17, 2022

So pretty good speed-ups from this PR, in the end. But I think the most revealing bits of this benchmarking, for me, are:

  • We should seriously consider running CI builds for PRs as -O0 until it's time to merge to main.
  • Our 80-core ARM64 builder is significantly slower than I expected.
  • Can we just convince the world to switch to Apple Silicon? The Mac times are outrageously good, even on our low-end laptops.

@dhess dhess added this pull request to the merge queue Aug 17, 2022
@georgefst georgefst removed this pull request from the merge queue due to a manual request Aug 17, 2022
brprice
brprice reviewed Aug 17, 2022
View reviewed changes
primer/src/Primer/Core.hs Outdated
Comment on lines 468 to 473
data ValCon = ValCon
{ valConName :: ValConName
, valConArgs :: [Type' ()]
}
deriving (Eq, Show, Data, Generic)
deriving (FromJSON, ToJSON) via PrimerJSON ValCon
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that ValCon should move with typedefs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, moved.

@georgefst georgefst force-pushed the georgefst/split-core-mod branch 2 times, most recently from ff67139 to 2b399af Compare August 18, 2022 10:43
@georgefst
refactor: Split up Primer.Core module
8df99e0 
The original intention here was to be able to put `Primitives.defType` somewhere more sensible. But as a bonus, this _should_ result in better compilation times, particularly incrementally.

We also take the opportunity to simplify the definition of `primConName`, now that it is in a module where it can access `tChar` etc.
@georgefst georgefst force-pushed the georgefst/split-core-mod branch from 2b399af to 8df99e0 Compare August 18, 2022 10:44
@georgefst georgefst enabled auto-merge August 18, 2022 10:44
@georgefst georgefst added this pull request to the merge queue Aug 18, 2022
Merged via the queue into main with commit 09022b5 Aug 18, 2022
@georgefst georgefst deleted the georgefst/split-core-mod branch August 18, 2022 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brprice brprice brprice approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@georgefst @dhess @brprice