-
Notifications
You must be signed in to change notification settings - Fork 36
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
Speed up changesUponFlattening #100
Conversation
0ee3916
to
fb35079
Compare
I believe the property should be
Does this look right @quchen? I could add a property test for this. |
Ping @quchen. This could help alleviate some performance issues in |
@@ -553,7 +553,7 @@ changesUponFlattening :: Doc ann -> Maybe (Doc ann) | |||
changesUponFlattening = \doc -> case doc of | |||
FlatAlt _ y -> Just (flatten y) | |||
Line -> Just Fail | |||
Union x _ -> changesUponFlattening x <|> Just x | |||
Union x _ -> Just 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.
Another idea:
Union x _ -> Just x | |
Union x _ -> 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.
Ah, no. This causes the fusion tests to fail with the classic
»SFail« must not appear in a rendered »SimpleDocStream«.
I'm not quite sure why though. Proper shrinking and diagnostic output would help… :/
I have written a test for this on top of #96 at master...sjakobi:99-group-tests. At least for the |
Since the first Union alternative was produced by the previous call to to changesUponFlattening, it seems unnecessary to scrutinize it again. This speeds up some prettyprinter-heavy applications in dhall by 20 to 42%. This assumes some kind of idempotence in changesUponFlattening. Fixes quchen#99.
fb35079
to
55469a6
Compare
While looking at some > diag $ group . group . group $ hardline
Union Fail (Union Fail (Union Fail Line)) In the layouter, this will still be slow since any overly wide This patch would fix that: @@ -523,6 +523,7 @@ hardline = Line
-- use of it.
group :: Doc ann -> Doc ann
-- See note [Group: special flattening]
+group x@Union{} = x
group x = case changesUponFlattening x of
Nothing -> x
Just x' -> Union x' x And maybe we can even do something smarter and look whether there's anything flattenable before we reach a first |
Superseded by #116. |
Since the first Union alternative was produced by the previous
call to to changesUponFlattening, it seems unnecessary to
scrutinize it again.
This speeds up some prettyprinter-heavy applications in dhall
by 20 to 42%.
This assumes some kind of idempotence in changesUponFlattening.
Fixes #99.