Skip to content

api: Switch to fused json utf8 decoder for parsing response #695

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

Merged
merged 1 commit into from
May 22, 2024

Conversation

rajveermalviya
Copy link
Member

Switch to a fused json+utf8 decoder, a more efficient path than decoding them individually. This implementation was made even faster recently in dart-lang/sdk@1e6c9b0.

In my local testing it reduces decoding time of response (excludes downloading time) from /register call from 1.40s to 0.78s resulting in ~45% improvement, (but still seems too high).

@gnprice
Copy link
Member

gnprice commented May 22, 2024

Neat! That upstream patch is fun, too.

In my local testing it reduces decoding time of response (excludes downloading time) from /register call from 1.40s to 0.78s resulting in ~45% improvement, (but still seems too high).

Can you post details of how you measured that? (E.g. with output of git diff, to show any code you changed locally.) That'd help make sure we're on the same page as to what's covered by the measurement.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks! Small comments.

@@ -7,6 +7,14 @@ import '../log.dart';
import '../model/localizations.dart';
import 'exception.dart';

/// A fused json + utf8 decoder.
Copy link
Member

Choose a reason for hiding this comment

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

Prose should use standard orthography:

Suggested change
/// A fused json + utf8 decoder.
/// A fused JSON + UTF-8 decoder.

even though identifiers often don't.

(Same thing goes for the commit message.)

/// a fast-path present in VM and WASM standard library implementations.
///
/// [1]: https://github.com/dart-lang/sdk/blob/6c7452ac1530fe6161023c9b3007764ab26cc96d/sdk/lib/_internal/vm/lib/convert_patch.dart#L55
final jsonUtf8 = const Utf8Decoder().fuse(const JsonDecoder());
Copy link
Member

Choose a reason for hiding this comment

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

nit: let's name it so it has a main noun that gives some hint as to what sort of thing this is:

Suggested change
final jsonUtf8 = const Utf8Decoder().fuse(const JsonDecoder());
final jsonUtf8Decoder = const Utf8Decoder().fuse(const JsonDecoder());

The shorter name indicates that it has something to do with JSON and with UTF-8, but that leaves its real meaning a bit of a mystery — the same name could just as well mean an encoder, for example.

@rajveermalviya
Copy link
Member Author

Can you post details of how you measured that?

Sure, here's the benchmark code: https://gist.github.com/rajveermalviya/46b4404c397b436769efb92bde6b9fa5
It uses benchmark_harness package to setup the benchmark, and embed package to embed the json file in the binary.

The res.json file was downloaded via following curl (copied from flutter dev tools networking panel):

curl --location --request POST 'https://chat.zulip.org/api/v1/register' \
    --header 'user-agent: ZulipFlutter' \
    --header 'content-type: application/x-www-form-urlencoded; charset=utf-8' \
    --header 'accept-encoding: gzip' \
    --header 'content-length: 290' \
    --header 'authorization: Basic [XXXX]' \
    --header 'host: chat.zulip.org' \
    --data-raw 'apply_markdown=true&slim_presence=true&client_gravatar=false&client_capabilities=%7B%22notification_settings_null%22%3Atrue%2C%22bulk_message_deletion%22%3Atrue%2C%22user_avatar_url_field_optional%22%3Afalse%2C%22stream_typing_notifications%22%3Afalse%2C%22user_settings_object%22%3Atrue%7D' \
    --compressed > ~/Downloads/res.json

@rajveermalviya
Copy link
Member Author

@gnprice Thanks for the review, pushed a new revision!

@rajveermalviya rajveermalviya requested a review from gnprice May 22, 2024 05:23
@gnprice gnprice added the integration review Added by maintainers when PR may be ready for integration label May 22, 2024
@gnprice
Copy link
Member

gnprice commented May 22, 2024

Sure, here's the benchmark code: https://gist.github.com/rajveermalviya/46b4404c397b436769efb92bde6b9fa5 It uses benchmark_harness package to setup the benchmark, and embed package to embed the json file in the binary.

The res.json file was downloaded via following curl (copied from flutter dev tools networking panel):

Interesting, thanks!

OK, so it's a benchmark comparing the two versions of basically just the one line changed here: decoding UTF-8 and JSON. And it runs 10 iterations and prints the average time.

What hardware were you running it on? (Specifically, what type of CPU?) That's the other piece of information that's key for interpreting this sort of benchmark result.

If the exact results of the measurements were important, we'd want to be sure to do them on a physical phone — the performance behavior is potentially different from a desktop or laptop, so that something helpful on one can be unhelpful on another. Here, though, there's no significant cost in code complexity, and we're doing something that clearly in principle should be better, so there's no need to deal with the logistics of that.


Meanwhile: looks good, and I'll merge. With one tweak in the commit message:

    -    api: Switch to fused JSON + UTF8 decoder for parsing response
    +    api: Switch to fused JSON + UTF-8 decoder for parsing response

@gnprice gnprice merged commit cf5218c into zulip:main May 22, 2024
1 check passed
@rajveermalviya rajveermalviya deleted the api-json-fuse branch May 23, 2024 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants