Skip to content
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

JSON.parse() performance #811

Open
1 task done
scarlac opened this issue Sep 6, 2022 · 24 comments
Open
1 task done

JSON.parse() performance #811

scarlac opened this issue Sep 6, 2022 · 24 comments

Comments

@scarlac
Copy link

scarlac commented Sep 6, 2022

Bug Description

TL;DR: JSON.parse() is 3x slower on Hermes than JSC.

| Engine                  | Method                 | Result |
|-------------------------+------------------------+--------|
| Hermes v0.12.0          | parseJson median       |   50.0 |
| Hermes v0.12.0          | parseJson avg          |   54.5 |
| JSC                     | parseJson median       |   15.0 |
| JSC                     | parseJson avg          |   16.6 |
| NodeJS v16.13.2         | parseJson median       |   15.0 |
| NodeJS v16.13.2         | parseJson avg          |   15.4 |

Porting a React Native app to use Hermes can cause performance regressions depending on use case.
I have narrowed it down to large JSON payloads being loaded. Specifically, JSON.parse() is much slower in Hermes than JSC. JSC is 70% faster than Hermes / Hermes is ~3x slower than JSC. This is significant for our use case and blocks us from migrating.

It's probably fair to say that parsing JSON data from a string is a very common use case. The performance regression (compared to JSC) is severe enough that we cannot use Hermes in production. We have tried 3 times and gotten complaints from users.

  • I have run gradle clean and confirmed this bug does not occur with JSC

Hermes version: v0.12.0
React Native version (if any): 0.68.2
OS version (if any): Android 12.0
Platform (most likely one of arm64-v8a, armeabi-v7a, x86, x86_64): arm64-v8a

Steps To Reproduce

  1. Call JSON.parse(largePayloadString)
  2. Compare with JavaScriptCore and NodeJS

Code Example:

https://github.com/scarlac/benchmark-fs#command-line-test-cli-testjs

The repo contains several performance tests but for the purpose of this ticket, we should focus on the cli-test.js file which can be run with the Hermes CLI tool:
hermes cli-test.js

NodeJS performs similar to JSC. It'd be amazing to have Hermes at that level as well.

The Expected Behavior

Performance should be similar (🤞 or better) than JSC.

@scarlac scarlac added the bug Something isn't working label Sep 6, 2022
@tmikov tmikov added performance and removed bug Something isn't working labels Sep 6, 2022
@tmikov
Copy link
Contributor

tmikov commented Sep 6, 2022

I was able to repro this with the file at the link.

@fbmal7
Copy link
Contributor

fbmal7 commented Sep 20, 2022

Hi @scarlac, just wanted to update you that the team has started an investigation into this issue. We are optimistic that we can bring perf improvements to JSON.parse, to bring it on par to JSC/V8. We will be entering the experimentation phase soon to see what changes help. We will update this thread as we make progress.

I wanted to ask more about the kinds of JSON data you are actually processing. In the benchmark you provided, objects with the same keys & values are being constructed many times. Is that representative of your actual production workload? Do you expect that a lot of the data you are processing will have the same keys or values? If there's any other useful benchmarking tests for JSON parsing you can share, that would be helpful as well.

@fbmal7 fbmal7 closed this as completed Sep 20, 2022
@fbmal7 fbmal7 reopened this Sep 20, 2022
@scarlac
Copy link
Author

scarlac commented Sep 21, 2022

@fbmal7 love to hear it. The data we typically parse is more varied and deeper nested but I wanted to make the test case clear and simple to begin with.

in reality we see a lot of variety in the values. Eg GUIDs, long lists of photos with local file paths (formatted as a URI), metadata with numbers and strings, product names, etc.

@billouboq
Copy link

@fbmal7 One big use case for this is the usage of redux-persist which save and load data from JS to any database we want. This is done a lot of times even if it's throttled

facebook-github-bot pushed a commit that referenced this issue Oct 17, 2022
Summary: Parse plain strings efficiently in JSON.parse. This is one step towards closing the gap in JSON.parse perf of Hermes vs JSC/V8, as noted in [this](#811) GH issue.

Reviewed By: jpporto

Differential Revision: D40283500

fbshipit-source-id: aa7ac3ee6c05328f7747aed1d7bf7e856ba763a7
@fbmal7
Copy link
Contributor

fbmal7 commented Oct 17, 2022

@scarlac We just landed 40de6b5 which did bring a decent perf win. When I ran it locally against the benchmark you provided, it improved the speed by 33%. We still have a long way to go to match the perf of V8/JSC, but this was an easier optimization to land first.

@scarlac
Copy link
Author

scarlac commented Oct 17, 2022

Thanks @fbmal7 ! Any improvement we can make to JSON handling is a giant win for almost all apps. I can't think of any that don't use JSON heavily. From API responses to persistence layers, to wonky deep object cloning.
Will there be a binary with this I can play around with?

@fbmal7
Copy link
Contributor

fbmal7 commented Oct 17, 2022

We don't tend to cut releases for Hermes very often. Building Hermes from source is always an option, and it's not too complicated. If you just want to build hermes and run it on the command line, you can use these instructions.

@scarlac
Copy link
Author

scarlac commented Nov 4, 2022

For those wondering, it looks like RN 0.71.0 is currently targeting Hermes tag hermes-2022-11-03-RNv0.71.0-85613e1f9d1216f2cce7e54604be46057092939d which include the 33% improvement(s):
1900bad
40de6b5

AndreiCalazans pushed a commit to AndreiCalazans/hermes that referenced this issue Nov 14, 2022
Summary: Parse plain strings efficiently in JSON.parse. This is one step towards closing the gap in JSON.parse perf of Hermes vs JSC/V8, as noted in [this](facebook#811) GH issue.

Reviewed By: jpporto

Differential Revision: D40283500

fbshipit-source-id: aa7ac3ee6c05328f7747aed1d7bf7e856ba763a7
@fbmal7
Copy link
Contributor

fbmal7 commented Nov 14, 2022

Hi @scarlac, just wanted to update you again. To get better improvements past the simple one we already landed has been a lot more work than initially anticipated. However, I wanted to let you know the team is still looking at this issue! We are exploring optimizations such as:

  • Making the parser iterative
  • Parsing strings for object properties vs. all other strings differently
  • Minimizing the amount of times we loop over scanned strings
  • Creating a new internal fast path API to set the property on the JSObject we are building during parse

I'm not too sure on the timeline for all of this, but will keep the thread updated as we make progress.

@scarlac
Copy link
Author

scarlac commented Nov 14, 2022

Thanks @fbmal7 . I appreciate the details and the team's hard work on this! Sounds like it'll make for some great release notes on the RN blog :)

@fbmal7
Copy link
Contributor

fbmal7 commented Feb 23, 2023

@scarlac My favorite thread to update! I'm finally getting around to landing some of the changes I wrote a couple months back. Two optimizations just landed yesterday and will make their way into 0.72. They did not bring as big of a win as the previous optimization.

The next change I have planned is one of the ones I referenced above- only performing expensive hash computation+lookups on strings that are object keys. Currently we are always performing a hash lookup for all strings we are scanning in the parser. We don't differentiate parsing for string keys vs. string values.

@scarlac
Copy link
Author

scarlac commented Feb 23, 2023

@fbmal7 Love getting these updates. Really impactful for everyone using RN. Thanks again!

@henrymoulton
Copy link

https://twitter.com/radexp/status/1630581333318680577

@radex would be great to hear about your JSON parsing improvements :)

@radex
Copy link
Contributor

radex commented Mar 1, 2023

yes, yes, soon :)

@radex
Copy link
Contributor

radex commented Mar 4, 2023

Here's a proof of concept: #933

@henrymoulton
Copy link

henrymoulton commented Mar 17, 2023

Hey @fbmal7, the blog post from @radex was a really amazing deep dive into the JSON parsing improvements. It'd be interesting to hear any plans you have off the back of #933, know it's early but I'm kind of interested and excited about what this could turn into! 🥳

@darwinshameran
Copy link

We're seeing the same issue, reverted back to using JSC for now. Would love to see Hermes performance being on par with JSC.

@hakansaglam29
Copy link

We upgraded react native version from 0.69.5 to 0.71.13 and then we just started to get syntax error from JSON.parse(). After deactivating hermes again, the issue is resolved. We could not use hermes in production!

@scarlac
Copy link
Author

scarlac commented Sep 11, 2023

We upgraded react native version from 0.69.5 to 0.71.13 and then we just started to get syntax error from JSON.parse(). After deactivating hermes again, the issue is resolved. We could not use hermes in production!

This doesn't sound related to the performance issue. Can you specify what exactly broke and provide a json snippet or at least the error message? Otherwise it's not possible to investigate.

@hakansaglam29
Copy link

We upgraded react native version from 0.69.5 to 0.71.13 and then we just started to get syntax error from JSON.parse(). After deactivating hermes again, the issue is resolved. We could not use hermes in production!

This doesn't sound related to the performance issue. Can you specify what exactly broke and provide a json snippet or at least the error message? Otherwise it's not possible to investigate.

This is not a performance issue but I think it should be resolved together.

We were trying to parse a data like this:

"{"lorem":{"xx_version":"1.0.0.0","yy_version":"1.0.0.1","zz_build":"Jul 17 2022 14:35:23","kk_version":"1","hh_id":"tttt-b-c8a952066002","gg_id":"@XXXXXXX","cc_id":"686725B847A0"}}"

After switching to new react native version 0.71.13, we just started to get SyntaxError: Unexpected token ..., although it is in the appropriate format.

@tmikov
Copy link
Contributor

tmikov commented Sep 11, 2023

@hakansaglam29 I just tried your example and it parses with Hermes without an error.

@hakansaglam29
Copy link

@hakansaglam29 I just tried your example and it parses with Hermes without an error.

Yes. if you try this output directly, JSON.parse is working. But i am getting this output as a response and using with JSON. parse. After that it throws an error. I could not understand why. But somehow after disabling Hermes, it worked again.

@fbmal7
Copy link
Contributor

fbmal7 commented Sep 11, 2023

@hakansaglam29 We can't debug the issue until you provide working repro steps. You should also open a new issue as this doesn't have to do with JSON parsing performance. There could be other polyfills or changes that happen when you switch to Hermes that are actually the root cause of your issue, not JSON.parse.

@hakansaglam29
Copy link

@hakansaglam29 We can't debug the issue until you provide working repro steps. You should also open a new issue as this doesn't have to do with JSON parsing performance. There could be other polyfills or changes that happen when you switch to Hermes that are actually the root cause of your issue, not JSON.parse.

Thanks @fbmal7 i will open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants