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

Speed up changesUponFlattening #100

Closed

Conversation

sjakobi
Copy link
Collaborator

@sjakobi sjakobi commented Nov 19, 2019

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.

@sjakobi sjakobi force-pushed the sjakobi/99-changesUponFlattening branch from 0ee3916 to fb35079 Compare November 20, 2019 18:42
@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 20, 2019

This assumes some kind of idempotence in changesUponFlattening.

I believe the property should be

group x == group (group x)

Does this look right @quchen? I could add a property test for this.

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 30, 2019

Ping @quchen.

This could help alleviate some performance issues in dhall, so I'm very interested to hear what you think about this! :)

@@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another idea:

Suggested change
Union x _ -> Just x
Union x _ -> Nothing

Copy link
Collaborator Author

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… :/

@sjakobi
Copy link
Collaborator Author

sjakobi commented Dec 12, 2019

This assumes some kind of idempotence in changesUponFlattening.

I believe the property should be

group x == group (group x)

Does this look right @quchen? I could add a property test for this.

I have written a test for this on top of #96 at master...sjakobi:99-group-tests.

At least for the Docs generated there, render . group == render . group . group seems to hold.

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.
@sjakobi sjakobi force-pushed the sjakobi/99-changesUponFlattening branch from fb35079 to 55469a6 Compare January 19, 2020 22:19
@sjakobi
Copy link
Collaborator Author

sjakobi commented Jan 19, 2020

While looking at some Docs with the new Diag, I noticed that even with this patch, repeated groups build up trees of Unions, just a bit faster than before:

> diag $ group . group . group $ hardline 
Union Fail (Union Fail (Union Fail Line))

In the layouter, this will still be slow since any overly wide Doc will be checked repeatedly until we finally decide to go with the innermost second alternative.

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 Union

@sjakobi
Copy link
Collaborator Author

sjakobi commented Jan 22, 2020

Superseded by #116.

@sjakobi sjakobi closed this Jan 22, 2020
@sjakobi sjakobi deleted the sjakobi/99-changesUponFlattening branch January 22, 2020 16:16
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.

changesUponFlattening can be slow
1 participant