Skip to content

group: Split top-level FlatAlt#140

Merged
sjakobi merged 1 commit intomasterfrom
115-split-flatalt
Feb 8, 2020
Merged

group: Split top-level FlatAlt#140
sjakobi merged 1 commit intomasterfrom
115-split-flatalt

Conversation

@sjakobi
Copy link
Collaborator

@sjakobi sjakobi commented Jan 27, 2020

This results in a significant speedup in the usual dhall benchmark.

Includes #123.

Closes #115.

@sjakobi sjakobi changed the title group: Split top-level flatAlt group: Split top-level FlatAlt Jan 27, 2020
@sjakobi
Copy link
Collaborator Author

sjakobi commented Jan 28, 2020

Before merging this I would like to figure out how pipelines are impacted depending on whether they use layout{Pretty,Smart,Compact}.

FlatAlt a b -> case changesUponFlattening b of
Flattened b' -> Union b' a
AlreadyFlat -> Union b a
NeverFlat -> a
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should add some documentation that explains why Union and FlatAlt are treated specially.

@sjakobi
Copy link
Collaborator Author

sjakobi commented Feb 8, 2020

Here's a slightly different take that should be pretty fast with layoutCompact:

--- a/prettyprinter/src/Data/Text/Prettyprint/Doc/Internal.hs
+++ b/prettyprinter/src/Data/Text/Prettyprint/Doc/Internal.hs
@@ -518,11 +518,16 @@ hardline = Line
 -- See 'vcat', 'line', or 'flatAlt' for examples that are related, or make good
 -- use of it.
 group :: Doc ann -> Doc ann
--- See note [Group: special flattening]
-group x = case changesUponFlattening x of
-    Flattened x' -> Union x' x
-    AlreadyFlat  -> x
-    NeverFlat    -> x
+-- See note [Group: special flattening] -- TODO
+group x = case x of
+    Union{}     -> x
+    FlatAlt a b -> Union (flat b) a
+    _           -> Union (flat x) x
+  where
+    flat a = case changesUponFlattening a of
+        Flattened a' -> a'
+        AlreadyFlat  -> a
+        NeverFlat    -> Fail
 
 -- Note [Group: special flattening]
 --
@@ -563,6 +568,7 @@ changesUponFlattening :: Doc ann -> FlattenResult (Doc ann)
 changesUponFlattening = \doc -> case doc of
     FlatAlt _ y     -> Flattened (flatten y)
     Line            -> NeverFlat
+    Union Fail _    -> NeverFlat
     Union x _       -> Flattened x
     Nest i x        -> fmap (Nest i) (changesUponFlattening x)
     Annotated ann x -> fmap (Annotated ann) (changesUponFlattening x)
@@ -1823,6 +1829,7 @@ layoutWadlerLeijen
         -> SimpleDocStream ann -- ^ Choice A. Invariant: first lines should not be longer than B's.
         -> SimpleDocStream ann -- ^ Choice B.
         -> SimpleDocStream ann -- ^ Choice A if it fits, otherwise B.
+    selectNicer _ _ SFail y = y
     selectNicer lineIndent currentColumn x y = case pWidth of
         AvailablePerLine lineLength ribbonFraction
           | fits pWidth minNestingLevel availableWidth x -> x

That turns out to slow down prettyprinting in dhall (with layoutSmart) though.

Right now I believe it's better to optimize for layoutSmart and layoutPretty, mostly because I'm not sure whether anyone's using layoutCompact at all. This package is about pretty-printing after all, and layoutCompact's output isn't exactly pretty.

This results in a significant speedup in the usual dhall benchmark.

Includes #123.

Closes #115.
@sjakobi
Copy link
Collaborator Author

sjakobi commented Feb 8, 2020

For the record, this is the new output for the motivating examples from #115 and #120:

> diag $ group (flatAlt "Default" "Fallback")
Union ( Text 8 "Fallback" ) ( Text 7 "Default" )
> diag $ group . group . group $ "x" <> line
Union 
    ( Cat ( Char 'x' ) ( Char ' ' ) ) 
    ( Cat ( Char 'x' ) 
        ( FlatAlt Line ( Char ' ' ) )
    )

For the second example, we might still want to try pushing the Union behind the Char 'x' and removing the inner FlatAlt, if that makes things faster.

@sjakobi sjakobi merged commit 51ec1d1 into master Feb 8, 2020
@sjakobi sjakobi deleted the 115-split-flatalt branch February 8, 2020 14:05
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.

Why doesn't group "split" FlatAlts?

1 participant