-
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
changesUponFlattening can be slow #99
Labels
Comments
sjakobi
added a commit
to sjakobi/prettyprinter
that referenced
this issue
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 quchen#99.
sjakobi
added a commit
to sjakobi/prettyprinter
that referenced
this issue
Nov 20, 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 quchen#99.
sjakobi
added a commit
to sjakobi/prettyprinter
that referenced
this issue
Jan 19, 2020
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.
Closed
This was referenced Jan 21, 2020
sjakobi
added a commit
that referenced
this issue
Jan 22, 2020
The main idea of this patch is to represent `changesUponFlattening`'s 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. This can noticeably speed up the layout of documents that heavily that heavily use group. Fixes #99, fixes #112.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In dhall-lang/dhall-haskell#1496 (comment) Gabriel describes a case where rendering a type error takes multiple seconds.
You can reproduce the issue with the following command, after adding a type error, for example by removing the
Some
on this line.These are the top cost centers:
Note that
changesUponFlattening
takes nearly 20% of the time!So I had a closer look at this function and noticed this case:
prettyprinter/prettyprinter/src/Data/Text/Prettyprint/Doc/Internal.hs
Line 556 in 7da3b1d
Union
is introduced bygroup
whichdhall
apparently uses a lot: the same profile contains over 600000 entries forgroup
!prettyprinter/prettyprinter/src/Data/Text/Prettyprint/Doc/Internal.hs
Lines 526 to 530 in 7da3b1d
So I wondered: If
changesUponFlattening
is only used bygroup
, and theUnion
constructor is also only introduced bygroup
, why doeschangesUponFlattening
have to scrutinize it at all? So I tried this change:This shaves about 18% off the total time when profiling and about 42% when using the unprofiled executable (and I'm not sure where that difference comes from)!
Unfortunately it also breaks the fusion property tests, which now fail with the classic
I haven't figured out yet quite how it breaks
fuse
though!The text was updated successfully, but these errors were encountered: