Change group not to produce a Union when the document cannot be flattened by sjakobi · Pull Request #116 · haskell-prettyprinter/prettyprinter

sjakobi · 2020-01-21T01:38:25Z

Fixes #99, fixes #112.

Addresses one idea from #112.

sjakobi · 2020-01-21T01:50:11Z

Previously:

> diag $ group $ "x" <> hardline
Union 
    ( Cat ( Char 'x' ) Fail ) 
    ( Cat ( Char 'x' ) Line )

Now:

Cat ( Char 'x' ) Line

sjakobi · 2020-01-21T01:51:35Z

dhall's tasty testsuite still passes when built with this branch.

sjakobi · 2020-01-21T01:54:28Z

It seems that we'd no longer produce Fails at all! We might as well remove the constructor then?!

sjakobi · 2020-01-21T01:57:03Z

It seems that we'd no longer produce Fails at all!

Nope. flatten still returns Fail for Line. Maybe we can get rid of that though…

sjakobi · 2020-01-21T02:01:58Z

flatten still returns Fail for Line. Maybe we can get rid of that though…

If flatten would also produce a FlattenResult, I think that probably reduce laziness, probably in a bad way?!

sjakobi · 2020-01-21T02:24:18Z

If flatten would also produce a FlattenResult, I think that probably reduce laziness, probably in a bad way?!

At least layoutCompact, which doesn't care about a first Union alternative at all, would suffer

https://github.com/quchen/prettyprinter/blob/ff555e19b7b17a74f16e6fb062256a57dabe4d92/prettyprinter/src/Data/Text/Prettyprint/Doc/Internal.hs#L1858

sjakobi · 2020-01-21T02:31:15Z

prettyprinter/src/Data/Text/Prettyprint/Doc/Internal.hs

 -- if the document is static (e.g. contains only a plain 'Empty' node). See
 -- [Group: special flattening] for further explanations.
-changesUponFlattening :: Doc ann -> Maybe (Doc ann)
+changesUponFlattening :: Doc ann -> FlattenResult (Doc ann)


Maybe this would be a better name:

Suggested change

changesUponFlattening :: Doc ann -> FlattenResult (Doc ann)

tryFlatten :: Doc ann -> FlattenResult (Doc ann)

sjakobi · 2020-01-21T02:57:53Z

prettyprinter/src/Data/Text/Prettyprint/Doc/Internal.hs

-    Union x _       -> changesUponFlattening x <|> Just x
+    FlatAlt _ y     -> Flattened (flatten y)
+    Line            -> NeverFlat
+    Union x _       -> Flattened x


This also includes #100 / fixes #99, mostly because I couldn't be bothered to write an Alternative instance.

sjakobi · 2020-01-21T03:02:07Z

prettyprinter/src/Data/Text/Prettyprint/Doc/Internal.hs

+    | AlreadyFlat
+    -- ^ The input was already flat, e.g. a 'Text'.
+    | NeverFlat
+    -- ^ The input couldn't be flattened: It contained a 'Line' or 'Fail'.


The main idea of this PR is to represent the old Nothing result with two constructors: One to indicate that the result is already flat enough, the other to indicate that flattening is impossible, and which allows shortcuts.

sjakobi · 2020-01-21T03:59:45Z

This speeds up the case described in dhall-lang/dhall-haskell#1496 (comment) from ~11.9s to ~9.2s for me.

I'm pretty confident that this is a good change at this stage.

sjakobi · 2020-01-21T04:35:07Z

This would also fix the issue of repeated groups I mentioned in #100 (comment).

sjakobi · 2020-01-21T04:50:05Z

I wonder whether I should add some kind of test for this. This patch should be unobservable after rendering, so I guess I could use diag…

sjakobi · 2020-01-21T04:52:22Z

This would also fix the issue of repeated groups I mentioned in #100 (comment).

Oops, nope:

> diag $ group . group . group $ "x" <> line
Union 
    ( Cat ( Char 'x' ) ( Char ' ' ) ) 
    ( Union 
        ( Cat ( Char 'x' ) ( Char ' ' ) ) 
        ( Union 
            ( Cat ( Char 'x' ) ( Char ' ' ) ) 
            ( Cat ( Char 'x' ) 
                ( FlatAlt Line ( Char ' ' ) )
            )
        )
    )

EDIT: I have opened #120 to track this.

sjakobi · 2020-01-22T14:24:08Z

This speeds up the case described in dhall-lang/dhall-haskell#1496 (comment) from ~11.9s to ~9.2s for me.

And here's the corresponding profile (see #99 (comment) for the original):

	Wed Jan 22 15:15 2020 Time and Allocation Profiling Report  (Final)

	   dhall +RTS -p -RTS type --file /home/simon/src/dhall-kubernetes-charts/stable/jenkins/index.dhall --explain

	total time  =       14.08 secs   (14079 ticks @ 1000 us, 1 processor)
	total alloc = 36,050,760,208 bytes  (excludes profiling overheads)

COST CENTRE               MODULE                                             SRC                                                                         %time %alloc

getDecodeAction           Codec.CBOR.Decoding                                src/Codec/CBOR/Decoding.hs:311:1-55                                          51.0   65.6
prettyCharacterSet        Dhall.Pretty.Internal                              src/Dhall/Pretty/Internal.hs:(512,1)-(1177,62)                                5.6    3.4
layoutWadlerLeijen        Data.Text.Prettyprint.Doc.Internal                 src/Data/Text/Prettyprint/Doc/Internal.hs:(1775,1)-(1843,60)                  5.5    3.0
unorderedTraverseWithKey_ Dhall.Map                                          src/Dhall/Map.hs:(647,1)-(648,65)                                             5.3    5.1
escapeLabel               Dhall.Pretty.Internal                              src/Dhall/Pretty/Internal.hs:(428,1)-(433,34)                                 4.0    2.1
renderLazy                Data.Text.Prettyprint.Doc.Render.Terminal.Internal src/Data/Text/Prettyprint/Doc/Render/Terminal/Internal.hs:(108,1)-(148,60)    3.7    3.7
removeTrailingWhitespace  Data.Text.Prettyprint.Doc.Internal                 src/Data/Text/Prettyprint/Doc/Internal.hs:(1477,1)-(1537,119)                 2.3    1.1
hashExpression            Dhall.Import                                       src/Dhall/Import.hs:(1133,1)-(1134,67)                                        2.2    2.4
styleToRawText            Data.Text.Prettyprint.Doc.Render.Terminal.Internal src/Data/Text/Prettyprint/Doc/Render/Terminal/Internal.hs:(278,1)-(304,29)    1.8    2.0
deserialiseIncremental    Codec.CBOR.Read                                    src/Codec/CBOR/Read.hs:(165,1)-(167,46)                                       1.7    1.1
layoutSmart               Data.Text.Prettyprint.Doc.Internal                 src/Data/Text/Prettyprint/Doc/Internal.hs:(1741,1)-(1767,59)                  1.6    0.2
changesUponFlattening     Data.Text.Prettyprint.Doc.Internal                 src/Data/Text/Prettyprint/Doc/Internal.hs:(567,1)-(607,21)                    1.5    1.1
csi                       System.Console.ANSI.Codes                          src/System/Console/ANSI/Codes.hs:77:1-75                                      1.4    2.7
toBuilder                 Codec.CBOR.Write                                   src/Codec/CBOR/Write.hs:(84,1)-(85,57)                                        1.0    1.0

time: 1.5% (down from 19.3%)
allocations: 1.1% (down from 22.6%)

This should speed up some pretty-printing tasks. See haskell-prettyprinter/prettyprinter#116 (comment) for some numbers.

sjakobi added 4 commits January 21, 2020 02:37

Change group to reveal failing Union alternatives more prominently

6f8da2e

Addresses one idea from #112.

Fail is Unflattenable

cad6a23

Better names?

f5362d4

Return x instead of (Union Fail x)

821f286

This addresses #112 (comment).

sjakobi changed the title ~~WIP: Change group to reveal failing Union alternatives more prominently~~ WIP: Change group not to produce a Union when the document cannot be flattened Jan 21, 2020

sjakobi commented Jan 21, 2020

View reviewed changes

sjakobi added 3 commits January 21, 2020 03:49

Better names, docs

7c4551a

Wibble

5a40e84

Save space

b1d20da

sjakobi commented Jan 21, 2020

View reviewed changes

sjakobi changed the title ~~WIP: Change group not to produce a Union when the document cannot be flattened~~ Change group not to produce a Union when the document cannot be flattened Jan 21, 2020

sjakobi requested a review from quchen January 21, 2020 04:15

Markup

5531a77

sjakobi mentioned this pull request Jan 21, 2020

Unbounded layout of grouped Line fails #91

Closed

sjakobi mentioned this pull request Jan 22, 2020

Optimization potential #121

Open

sjakobi merged commit 3eacd62 into master Jan 22, 2020

sjakobi deleted the 112-group-faster-fail branch January 22, 2020 16:16

sjakobi mentioned this pull request Jan 22, 2020

Speed up changesUponFlattening #100

Closed

sjakobi added a commit to dhall-lang/dhall-haskell that referenced this pull request Jan 22, 2020

Use prettyprinter-1.6

0df70b5

This should speed up some pretty-printing tasks. See haskell-prettyprinter/prettyprinter#116 (comment) for some numbers.

sjakobi added a commit to dhall-lang/dhall-haskell that referenced this pull request Jan 22, 2020

Use prettyprinter-1.6

083069f

This should speed up some pretty-printing tasks. See haskell-prettyprinter/prettyprinter#116 (comment) for some numbers.

sjakobi mentioned this pull request Jan 22, 2020

Use prettyprinter-1.6 dhall-lang/dhall-haskell#1641

Merged

sjakobi added a commit to dhall-lang/dhall-haskell that referenced this pull request Jan 22, 2020

Use prettyprinter-1.6 (#1641)

27df134

This should speed up some pretty-printing tasks. See haskell-prettyprinter/prettyprinter#116 (comment) for some numbers.

sjakobi mentioned this pull request Jan 22, 2020

group should be more idempotent #120

Closed

	changesUponFlattening :: Doc ann -> FlattenResult (Doc ann)
	tryFlatten :: Doc ann -> FlattenResult (Doc ann)

Conversation

sjakobi commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjakobi commented Jan 21, 2020

Uh oh!

sjakobi commented Jan 21, 2020

Uh oh!

sjakobi commented Jan 21, 2020

Uh oh!

sjakobi commented Jan 21, 2020

Uh oh!

sjakobi commented Jan 21, 2020

Uh oh!

sjakobi commented Jan 21, 2020

Uh oh!

sjakobi Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

sjakobi Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

sjakobi Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

sjakobi commented Jan 21, 2020

Uh oh!

sjakobi commented Jan 21, 2020

Uh oh!

sjakobi commented Jan 21, 2020

Uh oh!

sjakobi commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjakobi commented Jan 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

sjakobi commented Jan 21, 2020 •

edited

Loading

sjakobi commented Jan 21, 2020 •

edited

Loading