-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
Some more performance changes #236
base: master
Are you sure you want to change the base?
Conversation
MangoIV
commented
Aug 17, 2024
- max residency is down by ~ 50%
- removed two space leaks that only show up on very large files
- speed is approximately the same
- might be that it's even faster on really large files like hackage-packages.nix but I didn't run it often enough on to see
- did some refactorings
- fix a couple of space leaks - move all the extensiosn to cabal file - refactor some explictly recursive functions
I'm actually still thinking about the |
problem is that it probably fucks up But then again, Vector, too, and they don't even have pattern matching because it's such a bad idea... Maybe we should just rewrite the functions to something more idiomatic, at once... |
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.
Neat! This seems fine to me other than the semantic changes
. Megaparsec.parse Parser.file filename | ||
where | ||
-- f !x = layout $ maybe () (error . show) (unsafeNoThunks x) `seq` x | ||
f !x = layout x |
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.
What's this for?
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’m still a bit experimenting that’s why I converted back to draft. The function that’s commented out checks that some closure doesn’t contain thunks :)
unexpandSpacing' (Just n) _ | n < 0 = Nothing | ||
unexpandSpacing' _ [] = Just [] | ||
unexpandSpacing' n (txt@(Text _ _ _ t) : xs) = (txt :) <$> unexpandSpacing' (n <&> subtract (textWidth t)) xs | ||
unexpandSpacing' n (Spacing Hardspace : xs) = (Spacing Hardspace :) <$> unexpandSpacing' (n <&> subtract 1) xs | ||
unexpandSpacing' n (Spacing Space : xs) = (Spacing Hardspace :) <$> unexpandSpacing' (n <&> subtract 1) xs | ||
unexpandSpacing' n (Spacing Softspace : xs) = (Spacing Hardspace :) <$> unexpandSpacing' (n <&> subtract 1) xs | ||
unexpandSpacing' n (Spacing Break : xs) = unexpandSpacing' n xs | ||
unexpandSpacing' n (Spacing Softbreak : xs) = unexpandSpacing' n xs | ||
unexpandSpacing' _ (Spacing _ : _) = Nothing | ||
unexpandSpacing' n ((Group _ xs) : ys) = unexpandSpacing' n $ xs <> ys | ||
unexpandSpacing' _ Seq.Empty = Just [] | ||
unexpandSpacing' mn (x :<| xs) | ||
| Just n <- mn, n < 0 = Nothing | ||
| otherwise = | ||
let unexpandSubtract t = unexpandSpacing' (subtract t <$> mn) | ||
in case x of | ||
txt@(Text _ _ _ t) -> (txt :<|) <$> unexpandSubtract (textWidth t) xs | ||
(Spacing Hardspace) -> (Spacing Hardspace :<|) <$> unexpandSubtract 1 xs | ||
(Spacing Space) -> (Spacing Hardspace :<|) <$> unexpandSubtract 1 xs | ||
(Spacing Softspace) -> (Spacing Hardspace :<|) <$> unexpandSubtract 1 xs | ||
(Spacing Break) -> unexpandSpacing' mn xs | ||
(Spacing Softbreak) -> unexpandSpacing' mn xs | ||
(Spacing _) -> Nothing | ||
((Group _ ws)) -> unexpandSpacing' mn $ ws <> xs |
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.
This also changes the semantics a bit, because unexpandSpacing' (Just -1) Seq.Empty
now returns Just []
instead of Nothing
. This is likely the cause behind the changes in infinixbot/nixpkgs@2fb6f2c. Even if that would be better, let's have this PR only be internal refactorings.
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.
Sure, this was an accident, nice catch!
if p /= [] && (isSoftSpacing (head p) || isSoftSpacing (last p)) | ||
then error $ "group should not start or end with whitespace, use `group'` if you are sure; " <> show p | ||
else p | ||
case pretty x of | ||
((hp :<| _) :|> lp) | ||
| isSoftSpacing hp || isSoftSpacing lp -> | ||
error $ "group should not start or end with whitespace, use `group'` if you are sure; " <> show p | ||
_ -> p |
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.
This also changes semantics a bit, since the first pattern only applies with at least 2 elements, whereas before it also applied with just one.
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.
Oh you’re right. It was sure a bit late 🙃
Thanks for the review. I’ll apply fixes when I get back to this ❤️ |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/formatting-team-meeting-2024-08-20/50885/1 |