Skip to content

group: Add shortcut for previously grouped documents#123

Closed
sjakobi wants to merge 1 commit intomasterfrom
120-group-union
Closed

group: Add shortcut for previously grouped documents#123
sjakobi wants to merge 1 commit intomasterfrom
120-group-union

Conversation

@sjakobi
Copy link
Copy Markdown
Collaborator

@sjakobi sjakobi commented Jan 22, 2020

Fixes #120.

@sjakobi
Copy link
Copy Markdown
Collaborator Author

sjakobi commented Jan 22, 2020

This doesn't seem to speed up my dhall test case, but it prevents documents from needlessly blowing up, so I think it still makes sense.

@sjakobi
Copy link
Copy Markdown
Collaborator Author

sjakobi commented Jan 23, 2020

I noticed that this shortcut only works for documents that, in a previous group iteration, triggered the Flattened case – AlreadyFlat and NeverFlat don't get a Union constructor, so the result will be checked again by the next group. :(

This makes me wonder whether we should wrap the result of the AlreadyFlat and NeverFlat cases with e.g. Union Fail ... or a new Flat constructor to mark them as "grouped before".

Maybe this would be a useless optimization though – benchmarking necessary!

We should probably also document that group can be expensive, and should not be over-used.

@sjakobi
Copy link
Copy Markdown
Collaborator Author

sjakobi commented Jan 24, 2020

This makes me wonder whether we should wrap the result of the AlreadyFlat and NeverFlat cases with e.g. Union Fail ... or a new Flat constructor to mark them as "grouped before".

I gave this a try:

--- a/prettyprinter/src/Data/Text/Prettyprint/Doc/Internal.hs
+++ b/prettyprinter/src/Data/Text/Prettyprint/Doc/Internal.hs
@@ -527,8 +527,8 @@ group x = case x of
     Union{} -> x
     _ -> case changesUponFlattening x of
         Flattened x' -> Union x' x
-        AlreadyFlat  -> x
-        NeverFlat    -> x
+        AlreadyFlat  -> Union Fail x
+        NeverFlat    -> Union Fail x
 
 -- Note [Group: special flattening]
 --

dhall's testsuite still works, but I couldn't observe any performance gains either…

sjakobi added a commit that referenced this pull request Jan 27, 2020
This results in a significant speedup in the usual dhall benchmark.

Includes #123.
sjakobi added a commit that referenced this pull request Jan 27, 2020
This results in a significant speedup in the usual dhall benchmark.

Includes #123.

Closes #115.
@sjakobi
Copy link
Copy Markdown
Collaborator Author

sjakobi commented Feb 2, 2020

Superseded by #140.

@sjakobi sjakobi closed this Feb 2, 2020
@sjakobi sjakobi deleted the 120-group-union branch February 2, 2020 20:06
sjakobi added a commit that referenced this pull request Feb 8, 2020
This results in a significant speedup in the usual dhall benchmark.

Includes #123.

Closes #115.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

group should be more idempotent

1 participant