-
Notifications
You must be signed in to change notification settings - Fork 841
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
Cache exposed package modules in Pantry DB #4628
Conversation
subs/pantry/src/Pantry/Storage.hs
Outdated
|
||
loadExposedModulePackages | ||
:: (HasPantryConfig env, HasLogFunc env) | ||
=> SnapshotCacheHash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A guess would be that it would be more efficient to use SnapshotCacheId
here to avoid serializing a ByteString
each time.
src/Stack/Script.hs
Outdated
getPackagesFromModuleNames mns = do | ||
hash <- hashSnapshot | ||
withSnapshotCache hash mapSnapshotPackageModules $ \getModulePackages -> do | ||
mutableMapping <- mapMutablePackageModules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script
command does not allow for any mutable packages, so if desired for simplicity, I think you could simply add an assertion that there are no mutable packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know about that, is it documented somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, it's an implication of the requirement that there's no local config for stack script
, and when I wrote that documentation, we didn't have the mutable/immutable breakdown yet.
src/Stack/Script.hs
Outdated
compilerInfo <- getCompilerInfo wc | ||
let maybePliHash dep | PLImmutable pli <- dpLocation dep = | ||
Just $ immutableLocShaBs pli | ||
| otherwise = Nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly this could be an error "No mutable packages are allowed in the `script` command
src/Stack/Script.hs
Outdated
|
||
allExposedModules :: PD.GenericPackageDescription -> RIO EnvConfig [ModuleName] | ||
allExposedModules gpd = do | ||
-- FIXME add os, arch conditionals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify this FIXME
? It looks like you've already implemented these conditionals correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops
src/Stack/Build/Source.hs
Outdated
ghcOptions = map encodeUtf8 (cpGhcOptions dpCommon) | ||
haddocks = if cpHaddocks dpCommon then "haddocks" else "" | ||
hash = treeKeyToBs $ locationTreeKey pli | ||
hash = immutableLocShaBs pli | ||
return $ B.concat ([hash, haddocks] ++ flags ++ ghcOptions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible optimization: instead of using ByteString
s here, use Builder
s, which will more efficiently concatenate together. May not be worth the overhead of changing things.
@snoyberg I think I've addressed all the points you raised |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
Note: Documentation fixes for https://docs.haskellstack.org/en/stable/ should target the "stable" branch, not master.
Please include the following checklist in your PR:
Fixes #4536
Fixes #4624
I've switched to a script with some imports actually (otherwise there's an easy way to cheat by not getting any packages at all) :
And on my local machine I get the following results:
Thus we get only noticeable slowdown only when there's no module cache and otherwise
stack script
in this PR is only marginally slower than with the stable Stack. And with #4550 resolved we could get closer to the stable version or even get faster than it.