search

quchen / prettyprinter

A modern, extensible and well-documented prettyprinter.
BSD 2-Clause "Simplified" License
293 stars 34 forks source link

Make 'Terminal.renderLazy' lazy #176

Closed georgefst closed 4 years ago

georgefst commented 4 years ago

Fixes #175.

This is the non-monadic implementation, as requested by @sjakobi, which means a bit of extra parameter passing, but no big difference.

Any suggestions on how to benchmark this?

sjakobi commented 4 years ago

Any suggestions on how to benchmark this?

Good question! I guess a somewhat lasting solution would be to port the large-output benchmark from prettyprinter and add some annotations. No need to include all the different layouters though. Just using the fastest one should be enough. That's probably layoutCompact.

Does that sound reasonable?

georgefst commented 4 years ago

Does that sound reasonable?

Certainly does. I probably won't get around to it until next week though.

georgefst commented 4 years ago

Well... We actually seem to be looking at a more than 2x speedup.

Although, the benchmark data doesn't currently actually contain any annotations. I may add some and see what happens.

sjakobi commented 4 years ago

Well... We actually seem to be looking at a more than 2x speedup.

Wow! Any idea why?

Although, the benchmark data doesn't currently actually contain any annotations. I may add some and see what happens.

Yes, please do!

georgefst commented 4 years ago

Wow! Any idea why?

Not really. All I'd say is that the old implementation is in ST, seemingly in order to mirror the implementation of renderIO, which maybe introduces a significant overhead. Not sure as I've never used it, but it's certainly not the obvious approach.

georgefst commented 4 years ago

I've played around with this a bit, and adding annotations only amplifies the difference. As does using renderStrict, for some reason, even though it's still just toStrict . renderLazy.

The question now is which benchmarks are worth keeping around. We've got a lot of choices:

And using annotations also means using an algorithm other than layoutCompact - I'm not sure which would be best. (Side note - it's really not obvious from the docs that layoutCompact strips annotations.)

sjakobi commented 4 years ago

I've played around with this a bit, and adding annotations only amplifies the difference. As does using renderStrict, for some reason, even though it's still just toStrict . renderLazy.

Interesting. Have you looked at the Core to get more insight into the reason for the performance difference?

The question now is which benchmarks are worth keeping around. We've got a lot of choices:

  • renderStrict, renderLazy

renderLazy should be sufficient IMHO

  • comparison with ansi-wl-pprint

I don't care too much about that, although it's a nice baseline.

It might be nice to include prettyprinter's renderLazy as another baseline though.

  • light annotations, heavy annotations, none

Heavy annotations seem most interesting to me.

And using annotations also means using an algorithm other than layoutCompact - I'm not sure which would be best. (Side note - it's really not obvious from the docs that layoutCompact strips annotations.)

Oops! I didn't know that. Let's use layoutPretty then.

georgefst commented 4 years ago

I've updated the benchmark with layoutPretty defaultLayoutOptions, and lots of annotations, using Text.renderLazy as the baseline.

Have you looked at the Core to get more insight into the reason for the performance difference?

Nope. I don't have any experience with reading Core, and personally I'm happy to just defer to the magical black box that is GHC on this one.

sjakobi commented 4 years ago

Nice work, @georgefst! Could you paste the benchmark results with the old and new implementations here?

Regarding correctness: I'm unsure how much we can rely on the few doctests in prettyprinter-ansi-terminal. Have you tested this code in other contexts? Would the pretty-simple tests be robust enough to detect any issues?

georgefst commented 4 years ago

@sjakobi Well, there's:

So I'm reasonably confident.

georgefst commented 4 years ago

Old implementation:

benchmarking prettyprinter-ansi-terminal ... took 151.4 s, total 56 iterations
benchmarked prettyprinter-ansi-terminal
time                 2.827 s    (2.543 s .. 3.045 s)
                     0.988 R²   (0.972 R² .. 0.998 R²)
mean                 2.663 s    (2.499 s .. 2.790 s)
std dev              281.3 ms   (205.9 ms .. 378.3 ms)
variance introduced by outliers: 29% (moderately inflated)

New version:

benchmarking prettyprinter-ansi-terminal ... took 65.21 s, total 56 iterations
benchmarked prettyprinter-ansi-terminal
time                 1.242 s    (1.028 s .. 1.558 s)
                     0.941 R²   (0.892 R² .. 0.991 R²)
mean                 1.202 s    (1.112 s .. 1.326 s)
std dev              182.8 ms   (114.6 ms .. 268.5 ms)
variance introduced by outliers: 48% (moderately inflated)
georgefst commented 4 years ago

@sjakobi The review comments I haven't replied to all relate to things that were copy-pasted from the benchmark from the core library. I'm happy to implement all your suggestions, unless you can see any reason not to.

Presumably any backwards compatibility issues would be caught by CI?

georgefst commented 4 years ago

Presumably any backwards compatibility issues would be caught by CI?

By which I mean, specifically, compiling with old GHCs.

sjakobi commented 4 years ago

Yeah, we build the benchmarks in a few CI jobs, and I don't really care about the compatibility of the benchmarks with older GHC versions. So if you could clean up these wibbles a bit, that would be appreciated.

@quchen Modulo the mentioned wibbles, this looks ready to go to me. Would you like to review, too?

quchen commented 4 years ago

LGTM. The new code is much prettier and easier to understand than the ST-based version as well, and if the benchmark shows an improvement it’s a good change.

sjakobi commented 4 years ago

What's the status now, @georgefst? Is this ready?! :)

Have the benchmark results changed?

sjakobi commented 4 years ago

Oh, BTW, a question for a suspected native English speaker: Is "wibble" a proper word and appropriate where I used it here? I think I saw SPJ use it but I'm actually not sure whether I'm using it right. :)

georgefst commented 4 years ago

What's the status now, @georgefst? Is this ready?! :)

Yes it's ready, unless we care about the one remaining review comment. I'd be happy to leave it as-is, seeing as it's no worse than the existing benchmark in the core library.

Have the benchmark results changed?

Slight speedup with -O2 - around 20% for the new implementation, about 5% with the old one.

georgefst commented 4 years ago

wibble

I can't find any actual definition of it, but somehow I instantly knew what you meant. And I can't think of a good alternative, although "quibble" and "foible" are somewhat related. English is a silly language, and I've noticed SPJ is a fairly creative user of it.

Anyway, it amused me because it reminded me of a scene from a classic TV show, which is, as it happens, what almost all references to "wibble" on the web seem to refer to.

sjakobi commented 4 years ago

Thanks for all the work, @georgefst, and also the explanation for "wibble"! :)

sjakobi commented 4 years ago

FYI regarding the release: https://github.com/quchen/prettyprinter/issues/181