Skip to content

Conversation

@Yuras
Copy link
Contributor

@Yuras Yuras commented Apr 4, 2020

This PR replaces attoparsec with non-backtracking parser scanner, which improves performance by 20%-40% (see the benchmark results below.) It's WIP yet, I'd like to hear people's opinion on that.

Issues

There are two main issues with this change:

Backward compatibility

Since aeson exposes attoparsec parses from Data.Aeson.Parser module, replacing implementation will affect the API.

The possible solution is to convert parser based on scanner to one based on attoparsec using toAtto combinator from here. It requires this PR to be merged though. Note that toAtto preserves the speedup, i.e. parser implemented using scanner and then converted to attoparsec is still faster then one written directly in attoparsec.

Alternatively we can expose both implementations for some period of time.

Error messages

The error messages from the new parser lacks context. Here is a fragment of the output from the ErrorMessages test case:

        --- Error in $: not enough input. Expecting ':'
        --- Error in $: not enough input. Expecting object value
        --- Error in $: not enough input. Expecting ',' or '}'
        +++ Error in $: No more input
        +++ Error in $: unexpected end of input
        +++ Error in $: No more input

The reason is that scanner doesn't have a combinator similar to <?> from attoparsec. It's possible to add it, but it affects performance negatively.

Note that the error messages from attoparsec doesn't contain the error location, it makes them less useful. With scanner with can add error location to the error messages, it will make errors more useful.

Benchmarks

I used the bench-parse.py for benchmarking. Here are the results on my computer. Before:

    json-data/twitter1.json :: 60000 times
      0.626 seconds, 95806 parses/sec, 78.120 MB/sec
    json-data/twitter1.json :: 60000 times
      0.599 seconds, 100216 parses/sec, 81.716 MB/sec
    json-data/twitter1.json :: 60000 times
      0.614 seconds, 97689 parses/sec, 79.655 MB/sec
0.8 KB: 100217 msg\/sec (81.7 MB\/sec)
    json-data/twitter10.json :: 13000 times
      0.636 seconds, 20430 parses/sec, 128.477 MB/sec
    json-data/twitter10.json :: 13000 times
      0.624 seconds, 20830 parses/sec, 130.995 MB/sec
    json-data/twitter10.json :: 13000 times
      0.637 seconds, 20412 parses/sec, 128.364 MB/sec
6.4 KB: 20831 msg\/sec (131.0 MB\/sec)
    json-data/twitter20.json :: 7500 times
      0.763 seconds, 9823 parses/sec, 113.058 MB/sec
    json-data/twitter20.json :: 7500 times
      0.742 seconds, 10105 parses/sec, 116.303 MB/sec
    json-data/twitter20.json :: 7500 times
      0.742 seconds, 10103 parses/sec, 116.280 MB/sec
11.8 KB: 10105 msg\/sec (116.3 MB\/sec)
    json-data/twitter50.json :: 2500 times
      0.699 seconds, 3575 parses/sec, 108.937 MB/sec
    json-data/twitter50.json :: 2500 times
      0.703 seconds, 3554 parses/sec, 108.313 MB/sec
    json-data/twitter50.json :: 2500 times
      0.706 seconds, 3540 parses/sec, 107.879 MB/sec
31.2 KB: 3575 msg\/sec (108.9 MB\/sec)
    json-data/twitter100.json :: 1000 times
      0.662 seconds, 1509 parses/sec, 90.703 MB/sec
    json-data/twitter100.json :: 1000 times
      0.679 seconds, 1473 parses/sec, 88.514 MB/sec
    json-data/twitter100.json :: 1000 times
      0.653 seconds, 1530 parses/sec, 91.961 MB/sec
61.5 KB: 1531 msg\/sec (92.0 MB\/sec)
    json-data/jp10.json :: 4000 times
      0.592 seconds, 6756 parses/sec, 96.536 MB/sec
    json-data/jp10.json :: 4000 times
      0.600 seconds, 6661 parses/sec, 95.192 MB/sec
    json-data/jp10.json :: 4000 times
      0.597 seconds, 6702 parses/sec, 95.772 MB/sec
14.6 KB: 6756 msg\/sec (96.5 MB\/sec)
    json-data/jp50.json :: 1200 times
      0.644 seconds, 1862 parses/sec, 80.163 MB/sec
    json-data/jp50.json :: 1200 times
      0.627 seconds, 1914 parses/sec, 82.409 MB/sec
    json-data/jp50.json :: 1200 times
      0.646 seconds, 1858 parses/sec, 80.009 MB/sec
44.1 KB: 1914 msg\/sec (82.4 MB\/sec)
    json-data/jp100.json :: 700 times
      0.743 seconds, 941 parses/sec, 76.231 MB/sec
    json-data/jp100.json :: 700 times
      0.745 seconds, 939 parses/sec, 76.063 MB/sec
    json-data/jp100.json :: 700 times
      0.747 seconds, 937 parses/sec, 75.874 MB/sec
82.9 KB: 942 msg\/sec (76.2 MB\/sec)

After:

    json-data/twitter1.json :: 60000 times
      0.488 seconds, 122874 parses/sec, 100.190 MB/sec
    json-data/twitter1.json :: 60000 times
      0.480 seconds, 125054 parses/sec, 101.969 MB/sec
    json-data/twitter1.json :: 60000 times
      0.478 seconds, 125445 parses/sec, 102.287 MB/sec
0.8 KB: 125445 msg\/sec (102.3 MB\/sec)
    json-data/twitter10.json :: 13000 times
      0.459 seconds, 28311 parses/sec, 178.035 MB/sec
    json-data/twitter10.json :: 13000 times
      0.466 seconds, 27877 parses/sec, 175.307 MB/sec
    json-data/twitter10.json :: 13000 times
      0.462 seconds, 28159 parses/sec, 177.084 MB/sec
6.4 KB: 28311 msg\/sec (178.0 MB\/sec)
    json-data/twitter20.json :: 7500 times
      0.533 seconds, 14079 parses/sec, 162.038 MB/sec
    json-data/twitter20.json :: 7500 times
      0.527 seconds, 14239 parses/sec, 163.882 MB/sec
    json-data/twitter20.json :: 7500 times
      0.526 seconds, 14248 parses/sec, 163.981 MB/sec
11.8 KB: 14248 msg\/sec (164.0 MB\/sec)
    json-data/twitter50.json :: 2500 times
      0.498 seconds, 5019 parses/sec, 152.932 MB/sec
    json-data/twitter50.json :: 2500 times
      0.492 seconds, 5076 parses/sec, 154.690 MB/sec
    json-data/twitter50.json :: 2500 times
      0.496 seconds, 5035 parses/sec, 153.441 MB/sec
31.2 KB: 5077 msg\/sec (154.7 MB\/sec)
    json-data/twitter100.json :: 1000 times
      0.464 seconds, 2153 parses/sec, 129.369 MB/sec
    json-data/twitter100.json :: 1000 times
      0.463 seconds, 2158 parses/sec, 129.672 MB/sec
    json-data/twitter100.json :: 1000 times
      0.463 seconds, 2158 parses/sec, 129.690 MB/sec
61.5 KB: 2159 msg\/sec (129.7 MB\/sec)
    json-data/jp10.json :: 4000 times
      0.512 seconds, 7811 parses/sec, 111.625 MB/sec
    json-data/jp10.json :: 4000 times
      0.521 seconds, 7677 parses/sec, 109.707 MB/sec
    json-data/jp10.json :: 4000 times
      0.509 seconds, 7855 parses/sec, 112.243 MB/sec
14.6 KB: 7855 msg\/sec (112.2 MB\/sec)
    json-data/jp50.json :: 1200 times
      0.519 seconds, 2313 parses/sec, 99.590 MB/sec
    json-data/jp50.json :: 1200 times
      0.540 seconds, 2222 parses/sec, 95.687 MB/sec
    json-data/jp50.json :: 1200 times
      0.522 seconds, 2299 parses/sec, 98.975 MB/sec
44.1 KB: 2313 msg\/sec (99.6 MB\/sec)
    json-data/jp100.json :: 700 times
      0.623 seconds, 1123 parses/sec, 90.915 MB/sec
    json-data/jp100.json :: 700 times
      0.605 seconds, 1156 parses/sec, 93.613 MB/sec
    json-data/jp100.json :: 700 times
      0.600 seconds, 1166 parses/sec, 94.408 MB/sec
82.9 KB: 1166 msg\/sec (94.4 MB\/sec)

@phadej
Copy link
Collaborator

phadej commented Apr 4, 2020

It's good to verify my suspicion that attoparsec is not as fast as it could be.

One concern if/when we switch the main driver for ByteString -> ... Value, we have do decide what to do withjson :: attoparsec.Parser Value, as these aren't tested "by everything else" anymore.

I don't think it's worth depending on a new library. JSON grammar is so simple that the "faster than aeson" parser can be written by hand, pretty simple, and be vendored in aeson. (I have one for strict bytestrings, e.g. it doesn't produce Value though: it's not even a lot of code: https://github.com/phadej/saison/blob/master/src/Saison/Decoding/Parser.hs). I'd argue that we should duplicate the code and have separate versions for strict and lazy bytestrings. Strict one could be noticeably faster, as one doesn't need to be concerned about chunk boundaries (and this is why saison has only parser from strict ByteString atm).

I feel that aeson should be super strict about adding new dependencies, the scanner is not that complicated something like that could be internal to aeson if it helps (I guess it does for lazy bytestrings). Relatedly, I'd like to drop dlist dependency, as it's not that useful, but is additional dependency which thrashes my package cache.

Thanks for looking where aeson can be improved!

@phadej
Copy link
Collaborator

phadej commented Apr 4, 2020

Also let's first resolve #766, I'll hopefully get to it next weekend.

@Yuras
Copy link
Contributor Author

Yuras commented Apr 6, 2020

@phadej Creating a separate parser for strict bytestrings makes sense for me. And if we include scanner into aeson (or implement something similar), then we'll break an annoying dependency cycle between aeson, criterion and the benchmarks from scanner, so I support that too.

I'm not sure what you meant here:

One concern if/when we switch the main driver for ByteString -> ... Value, we have do decide what to do withjson :: attoparsec.Parser Value, as these aren't tested "by everything else" anymore.

Could you please elaborate?

To make sure we are on the same page: we'll wait for #766 and then we'll decide on this PR. Is it right?

@phadej
Copy link
Collaborator

phadej commented Apr 6, 2020

Could you please elaborate?

The attoparsec parsers on https://hackage.haskell.org/package/aeson-1.4.7.1/docs/Data-Aeson-Parser.html are not tested that much directly (I'm not sure if at all). I don't think removing them is a good idea, but we need some (direct) tests for them. This can be done while waiting for #766. E.g. duplicating the JSONTestSuite to run on each separate parser.

To make sure we are on the same page:

Yes, let's do #766 first.

@phadej
Copy link
Collaborator

phadej commented Apr 11, 2020

Using #772 one can now indeed see that this is faster:

Benchmark                              master  master2          master3          scanner           scanner2         
Examples/decode/github-issues/lazy    1.77e-3  1.78e-3  +0.43%  1.83e-3  +3.26%  1.55e-3  -12.41%   1.58e-3  -10.63%
Examples/decode/github-issues/strict  1.82e-3  1.72e-3  -5.27%  1.71e-3  -6.05%  1.52e-3  -16.26%   1.55e-3  -14.48%
Examples/decode/jp100/lazy            1.94e-3  1.98e-3  +1.93%  1.98e-3  +2.01%  1.79e-3   -7.90%   1.80e-3   -7.24%
Examples/decode/jp100/strict          1.92e-3  1.97e-3  +2.41%  1.95e-3  +1.32%  1.68e-3  -12.34%   1.72e-3  -10.59%
Examples/decode/twitter100/lazy       1.49e-3  1.50e-3  +0.38%  1.51e-3  +0.88%  1.28e-3  -14.53%   1.28e-3  -14.52%
Examples/decode/twitter100/strict     1.51e-3  1.57e-3  +3.86%  1.54e-3  +1.66%  1.30e-3  -14.13%   1.30e-3  -14.13%

my machine was noisy, but still, 10% speedup is quite nice!

(Note: #772 is not resolving #766, though it setups the scene quite well).

\w -> w /= DOUBLE_QUOTE && w /= BACKSLASH && not (testBit w 7)
w <- lookAhead'
case w of
DOUBLE_QUOTE -> Scanner.anyWord8 $> Text.decodeUtf8 s
Copy link
Contributor

Choose a reason for hiding this comment

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

Please try Text.decodeLatin1 here.

@ethercrow
Copy link
Contributor

Please revive this PR, it's amazing technology.

@Yuras
Copy link
Contributor Author

Yuras commented Feb 25, 2021

Please revive this PR, it's amazing technology.

I somehow missed your comment @ethercrow
The PR is blocked till we decide on backward compatibility. If this PR is merged, then we'll be able to make the change backward compatible, otherwise we need another plan.

@ethercrow
Copy link
Contributor

@Yuras getChunk was added.

@Yuras
Copy link
Contributor Author

Yuras commented Nov 25, 2021

@ethercrow Indeed, and I even released new version of scanner-attoparsec (I have no memory about it for some reason :) )

Now I'd like to hear from maintainers whether they agree with the proposed approach to backward compatibility. Namely, implement parser via scanner and provide attoparsec parser on top of it using toAtto. It might require bumping the lower bound for attoparsec to 0.14.1, which is a slight issue.

Also let's decide whether 10% speedup worth the efforts at all. Nowadays there is a number of JSON parsers on Hackage specifically designed for performance.

@Yuras
Copy link
Contributor Author

Yuras commented Nov 25, 2021

Ah, and vendoring custom JSON parser into aeson is also a very reasonable option! I think we have enough data to make a decision.
I might not have too much time to work on this PR in near future. I'll try to find time because it's an interesting task, but I can't promise. It's another thing to take into account when deciding.

@phadej
Copy link
Collaborator

phadej commented Feb 9, 2023

The ideas from here are in #996

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.

3 participants