realm / realm-js

Realm is a mobile database: an alternative to SQLite & key-value stores
https://realm.io
Apache License 2.0
5.62k stars 558 forks source link

Performance regression vs non-Hermes version? #4443

Closed liamjones closed 3 months ago

liamjones commented 2 years ago

Description

While investigating https://github.com/realm/realm-js/issues/4328 (which is now resolved), I noticed that performance seemed to have halved on the Hermes builds when importing several thousand objects (see https://github.com/realm/realm-js/issues/4328#issuecomment-1059743396).

Stacktrace & log output

10.20.0-beta.2:
Loaded 6066 Products in 13.883 seconds
Loaded 6066 Products in 14.27 seconds
Loaded 6066 Products in 13.969 seconds

10.20.0-beta.1:
Loaded 6066 Products in 13.923 seconds
Loaded 6066 Products in 13.829 seconds
Loaded 6066 Products in 13.909 seconds

10.13.0:
Loaded 6066 Products in 7.503 seconds
Loaded 6066 Products in 7.485 seconds
Loaded 6066 Products in 7.504 seconds

Can you reproduce a bug?

Yes, always

Reproduction Steps

I don't have time to create a repro right now (sorry!) - I'm just logging the ticket now so I don't forget. I will try and come back with a minimal repro in a week or two.

Version

10.20.0-beta.1 & beta.2

What SDK flavour are you using?

Local Database only

Are you using encryption?

No, not using encryption

Platform OS and version(s)

RN 0.66.3 using JSC (not Hermes!) - iPhone 13 Simulator w/ iOS 15.2, Android AVD Emulator running Android 9.0

tomduncalf commented 2 years ago

Thanks for the report @liamjones – we are aware there are regressions here and will be looking into them as part of the ongoing work on Hermes. We'll update you when we have some more info

liamjones commented 2 years ago

Thanks Tom!

fronck commented 2 years ago

@liamjones I'm working on profiling of the Hermes code. Can you share anything about the characteristics of the import you do? E.g.,

liamjones commented 2 years ago

Hi @fronck, sorry for the delayed reply, been a busy week!

I've cut down a minimal repro and sent it in to realm-help@mongodb.com so you can see the exact code we're running - look for email titled "Private repro for realm-js issues #4443 & #4328"

tomduncalf commented 1 year ago

Hi @liamjones, I've been able to do some investigation into this this week. It turns out that in the current version of the Hermes engine, it is always performing some timing of all JS/C++ calls, which adds quite considerable overhead in our case: https://github.com/facebook/hermes/blob/v0.11.0/API/hermes/hermes.cpp#L185-L194

With this timing disabled (I modified the Hermes source and compiled my own version of it), I see Realm performance on iOS increase by 2-2.5x in our performance benchmarks, which makes it comparable or faster than JSC for our basic data types.

I've raised this with Meta and they have said that “this is a known problem, and we’re planning to make these timers dynamically configurable (and off by default) in the next release”, so the good news is that hopefully this will be fixed in the next React Native release (and might improve performance of anything else which interacts with Hermes!).

The bad news is that there's not much you can do about it for now, unless you are willing to use a custom version of Hermes in your app (I can tell you how to do so if you are interested).

Some of our data type benchmarks are still slower on Hermes vs. JSC, so I'm looking into this to see if there's anything we can do. For reference, these are the results I'm seeing on iOS with my modified Hermes (a number greater than 1 means Hermes is faster, a number less than 1 means Hermes is slower):

Data type Hermes performance vs JSC
bool 1.07
int 1.00
float 1.13
double 1.15
string 1.00
decimal128 0.15
objectId 0.47
uuid 0.35
date 0.79
data 1.38
Car 0.78
bool[] 0.36
bool<> 0.38
bool{} 0.40
liamjones commented 1 year ago

Thanks for the info @tomduncalf ! Does this apply to my comparisons though? I was using the Hermes build of Realm but still with the JSC, not Hermes as the JS engine. Even then I was seeing this performance reduction.

tomduncalf commented 1 year ago

Oh interesting @liamjones, I totally missed that, sorry! I guess it would not apply in that case... I'll take a look and see if I can reproduce.

tomduncalf commented 1 year ago

By way of an update @liamjones, I've found that the way we are accessing the native Realm C++ objects in v11 via JSI performs quite a lot slower than the way we do it on master via JSC.

The JSI abstraction layer seems to add some cost, but it's mainly the difference between accessing an object property (which seems to be relatively slow) vs. accessing its "private data" (which is quick).

There is no way with JSI to interact directly with the "private data" of an object that I can see, but we are investigating whether there are other ways we can store and access these objects which might perform better. We'll keep you updated on our findings!

tradebulls commented 1 year ago

@tomduncalf @takameyer We are using a custom react native library already. We can modified the Hermes.cpp file and try it out in IOS. However, I am curious to know how could we achieve the same in Android versions. Can you please help?

HSReact commented 1 year ago

Hi @liamjones, I've been able to do some investigation into this this week. It turns out that in the current version of the Hermes engine, it is always performing some timing of all JS/C++ calls, which adds quite considerable overhead in our case: https://github.com/facebook/hermes/blob/main/API/hermes/hermes.cpp#L185-L194

With this timing disabled (I modified the Hermes source and compiled my own version of it), I see Realm performance on iOS increase by 2-2.5x in our performance benchmarks, which makes it comparable or faster than JSC for our basic data types.

I've raised this with Meta and they have said that “this is a known problem, and we’re planning to make these timers dynamically configurable (and off by default) in the next release”, so the good news is that hopefully this will be fixed in the next React Native release (and might improve performance of anything else which interacts with Hermes!).

The bad news is that there's not much you can do about it for now, unless you are willing to use a custom version of Hermes in your app (I can tell you how to do so if you are interested).

Some of our data type benchmarks are still slower on Hermes vs. JSC, so I'm looking into this to see if there's anything we can do. For reference, these are the results I'm seeing on iOS with my modified Hermes (a number greater than 1 means Hermes is faster, a number less than 1 means Hermes is slower):

Data type Hermes performance vs JSC bool 1.07 int 1.00 float 1.13 double 1.15 string 1.00 decimal128 0.15 objectId 0.47 uuid 0.35 date 0.79 data 1.38 Car 0.78 bool[] 0.36 bool<> 0.38 bool{} 0.40

@tomduncalf Hi Tom, thanks for this, do you happen to have performance benchmarks on read/write times, sync times (thats a big one)...many thanks

tomduncalf commented 1 year ago

@HSReact I don't have any benchmarks on those. The only place we would expect to see any change in performance characteristics is in the JS/C++ interop layer (previously JSC, now JSI/Hermes), so we think that benchmarking property access provides a pretty good proxy measurement for this.

Read/write/sync are all implemented purely in our C++ core, so we wouldn't expect to see any performance regression of the actual internal implementations of those features – but if the JS/C++ interop performance is reduced, you would see reduced read/write/sync throughput regardless as each read/write/sync needs to go between C++ and JS.

Once we solve the regressions as measured with the current test suite, I'd expect that to also solve any regressions seen in read/write/sync. However, it's a great suggestion that we should have tests to benchmark these operations more generally, and we will definitely be looking to expand our peformance testing suite in future.

mfbx9da4 commented 1 year ago

Definitely agree that having benchmarks is a great place to start.

Just to add to hermes/JSI branch performance issues, an empty write transaction is taking ~20ms as noted in #3709.

Also I'm finding that to iterate through a collection of about 500 items and access the keys on each item takes ~50ms 🤯 . (The objects in the collection are fairly small trivial objects). For context the same code takes ~1ms in plain JS, 50x faster.

Workarounds would be greatly appreciated, thanks.

tomduncalf commented 1 year ago

I'll document the workaround for when Hermes is enabled here in case you want to try it, but it's pretty ugly! There are probably ways to neaten it up (e.g. create a fork of RN/Hermes), but as this has already been fixed in Hermes master and so should be fixed in an upcoming RN version, I don't think it makes sense for us to try to officially support a workaround right now.

Essentially the steps are:

  1. Take Hermes 0.9.0 (used in the current RN version) and pull in https://github.com/facebook/hermes/commit/2a32afb225349c75f3aeb4ffca10f3c4daab6fad to allow disabling the timer
  2. Recompile Hermes from source
  3. Drop this compiled Hermes into your RN project, overwriting the existing one

In more detail – I figured these steps out from their CircleCI workflow and have reconstructed the steps from memory/my shell history, so apologies if I miss something, let me know if you get stuck.

For iOS:

  1. Clone my fork of the Hermes repo from https://github.com/tomduncalf/hermes
  2. Check out the td/timer-disabled branch, where I've cherry picked the commit from Hermes which disables the timer onto the v0.9.0 tag (which is what React Native needs)
  3. In the hermes clone directory, run ./utils/build-ios-framework.sh and wait ☕
  4. Copy the compiled xcframework from hermes/destroot/Library/Frameworks/Universal/hermes.xcframework to your project's ios/Pods/hermes-engine/destroot/Library/Frameworks/universal/hermes.xcframework, overwriting the existing one

For Android:

  1. Clone my fork of the Hermes repo from https://github.com/tomduncalf/hermes
  2. Check out the td/timer-disabled branch, where I've cherry picked the commit from Hermes which disables the timer onto the v0.9.0 tag (which is what React Native needs)
  3. Create a directory at the same level as your hermes clone called hermes_ws and cd hermes_ws
  4. Set an env var pointing to this dir:
    export HERMES_WS_DIR=`pwd`
  5. Create a symlink to hermes inside hermes_ws: ln -s ../hermes
  6. Configure the hermes compiler build: hermes/utils/build/configure.py ./build
  7. Build the hermes compiler: cmake --build ./build --target hermesc
  8. Build the hermes package:
    export ANDROID_SDK="$ANDROID_HOME"
    export ANDROID_NDK="$ANDROID_HOME/ndk-bundle"
    cd "$HERMES_WS_DIR/hermes/android" && ./gradlew githubRelease
  9. Unzip the build artefact, hermes_ws/build_android/distributions/hermes-runtime-android-v0.9.0.tar.gz
  10. In your React Native project directory, overwrite the files in node_modules/hermes-engine/android/ with the files you unzipped in step 9.
mfbx9da4 commented 1 year ago

Thanks for doing so though in my case Hermes is not enabled.

tomduncalf commented 1 year ago

Ah OK, we don't have a workaround for that case yet. I'll update this issue when we have a fix though

tradebulls commented 1 year ago

@tomduncalf A new update to React Native is available now. https://github.com/facebook/react-native/blob/main/CHANGELOG.md

Are the Hermes performance issue fixed in this RN release? Can we expect an update to realm with performance issue fixes with Hermes Enabled?

I guess with new update, the cost for JSI abstraction layer might be reduced. Thanks in advance

tomduncalf commented 1 year ago

Hey @tradebulls, thanks for letting me know – I'll take a look at the new release and see. They mention they are now building Hermes from source so hopefully this will pull in the recent changes in Hermes. I'll update you once I've had a chance to look.

tradebulls commented 1 year ago

Thanks for doing so. Looking forward to your reply

tomduncalf commented 1 year ago

It looks like this will require some work from our side to support, if I drop the new React Native into our Hermes branch I get a Hermes error. We will update you once we have some progress on this, it is a priority for us as we know these performance regressions are causing issues.

tomduncalf commented 1 year ago

Hey @tradebulls, unfortunately I'm still seeing a performance regression with the latest RN. I'm following up with Meta to see what we can do mitigate this.

tradebulls commented 1 year ago

@tomduncalf Thanks for doing so.

tomduncalf commented 1 year ago

I've confirmed that this change did not make it to RN 0.69, but should be in RN 0.70 which should hopefully be out fairly soon.

tradebulls commented 1 year ago

Ohhk...to have a new major version of RN to be released takes about 2+months. As RN0.69 is recently rolled out, they would be providing with minor version release for a while(Eg Rn 0.69.1 or RN 0.69.2). @tomduncalf any idea if they are planning to put it in minor release as it would be out soon...

tomduncalf commented 1 year ago

Obviously I can't make any commitments for the Meta team, but it sounded like they were hoping to get this out sooner than that

tradebulls commented 1 year ago

Ok.. Thanks for the update :)

tradebulls commented 1 year ago

@tomduncalf is the performance issue address in Rn 0.69.1? Something similar to performance fixes by RN, not really issue if this is related to our performance issue. Use monotonic clock for performance.now() (114d31feee)

tomduncalf commented 1 year ago

@tradebulls I think that's something different. The issue causing the regression is this timer code in Hermes which times every C++ <-> JS call, which results in a lot of extra CPU usage for Realm as we make a lot of these calls. There is a compile-time option to disable the timer (HERMESJSI_DISABLE_STATS_TIMER) but the current build of Hermes in React Native 0.69 did not have this flag enabled when they compiled it. We're expecting it to be fixed in the first RC of 0.70.

tradebulls commented 1 year ago

Ok.. Thanks for the clarity...Really appreciated :)

mfbx9da4 commented 1 year ago

Hey, any timeline on the non-Hermes situation? When can we expect an update about how long it will take to resolve?

tradebulls commented 1 year ago

@mfbx9da4 It seems RN 0.70.0 RC is out with the changelog. However no updates on HERMESJSI_DISABLE_STATS_TIMER disabled feature as of now. You can check the same here - https://github.com/facebook/react-native/releases/tag/v0.70.0-rc.0

mfbx9da4 commented 1 year ago

@tradebulls I am more curious about the non-Hermes case

tomduncalf commented 1 year ago

Hey @mfbx9da4, sorry the delay on this one. We have been looking into the non-Hermes regression but are yet to find a solution, however we are hoping that we can find a way to bypass the JSI layer for this "hot path" case and just call directly into JSC like we do in the master branch. I'll keep you updated as to how we get on with this.

mfbx9da4 commented 1 year ago

Have you been able to identify the cause at this stage or is the conclusion that the JSI isn't as fast as it is purported to be?

tomduncalf commented 1 year ago

@mfbx9da4 The issue is as described here: https://github.com/realm/realm-js/issues/4443#issuecomment-1143599640

Essentially the way we store the C++ object "associated with" a given JS object has had to change in order to use JSI, and the new way to retrieve that object is a lot slower, which as it's a very frequently called operation, leads to these performance issues. It's not really a problem with JSI itself, more the way our code is architected doesn't quite fit with how JSI would like it to be architected (where you expose C++ objects using the HostObject class, which we concluded wouldn't work for Realm).

We are looking into whether we can either somehow revert to the old JSC "private data" mechanism for accessing the C++ instance in the case where Hermes is not being used (this might depend on whether the appropriate internals of JSI can be accessed from outside or not), or otherwise whether we can e.g. implement some kind of object cache on our side.

tomduncalf commented 1 year ago

@liamjones @mfbx9da4 In case it is of interest, I posted an update on the performance regression situation, after a great deal of research, here: https://github.com/realm/realm-js/issues/3940#issuecomment-1238168460

liamjones commented 1 year ago

Thanks @tomduncalf! I appreciate all the time that has been dedicated to looking at this.

If it's primarily an issue with large numbers of inserts then I think, personally, our app should be okay until we can get onto Hermes. We only do a large batch of inserts on the first launch and most work after that is accessing a few records at a time.

tomduncalf commented 1 year ago

No problem @liamjones. I think it's an issue that could affect anything where you are doing large numbers of operations of any kind, because each operation needs to fetch the attached C++ object, which will be slower... but I'd hope this slowness is not observable in normal operations, and only really when you are doing a lot of operations at the same time. If you find otherwise please let us know.

It would be interesting for us to understand what some of the obstacles holding you back from moving to Hermes are, if you're able to share?

liamjones commented 1 year ago

It would be interesting for us to understand what some of the obstacles holding you back from moving to Hermes are, if you're able to share?

Sure!

The main initial blocker was Realm not being compatible and the conflict with react-native-reanimated. This was stopping us from testing Hermes at all with our app, we'd have had to strip Realm out/shim it and that wouldn't be particularly simple - it's a chunky app at over 215k lines of code (excluding node_modules).

Then, the various issues with the Hermes branch of Realm not working with the JSC meant I wasn't able to upgrade Realm first on the JSC, and check everything was happy, before tackling Hermes. The version of Hermes you needed in the betas also meant I would have to tackle an RN upgrade at the same time.

I didn't want to do all of these together as I thought it'd probably be difficult to work out which change resulted in which breakage post upgrade.

The issues with iOS Intl API support on Hermes (and the slowness of the polyfills) were a concern too though those look like they're mostly (all?) resolved now.

Beyond that, the remaining blocker is the time/complexity of upgrading so we can try Hermes vs business priorities on delivering some new app features this year (and we only have a small app dev team). We will need to upgrade RN, Realm, react-native-reanimated and Expo (migrating from unimodules to Expo modules) at the same time due to some version interdependencies. After all that I expect various other native modules to need some attention due to the RN upgrade. Then we can try Hermes! 😄

tomduncalf commented 1 year ago

Got it, thanks for the detailed explanation! As we are not really building apps ourselves any more, it's very useful to get feedback from people who are doing so, so we can understand how quickly the community are likely to e.g. move to Hermes.

stefoid commented 1 year ago

Hi what is the status of this issue? Id like to give some reports on performance issues with our RN app, using rn 71.3 and realm 11.5.1 (hermes enabled) - these issues are much worse for android than iOS

Using non-hermes realm 10.19.0 / RN 67, we see a 2x performance improvement in general performance (reads and writes) vs 11.5.1 / 71.3

Our app uses a lot (8 or 9) open filtered results sets to keep several lists and other UX elements 'live'

When there are only a few hundred records in the tables corresponding to these lists, everything runs acceptably But when there are 5K+ records in these tables, the app slows to a crawl. the lists lists draw slowly as the user scrolls, and if small writes are performed, the app pauses for seconds at a time!

Ive tried closing some of these live result sets and the performance of the app improves a lot.

So the summary is : not much data in the DB and lots of live results sets = OK lots of data in the DB and lots of live results sets = OK lots of data in the DB and lots of live results sets = BAD

And the above is made 2x worse by running the latest version of realm as opposed to 10.19.0

I really need to do something about this, and I need some more insight into the above so that I can come up with a decent solution for right now, and in the future.

thanks realm team!

HSReact commented 1 year ago

An update from the Realm team on performance improvements is welcome at this stage…

kneth commented 1 year ago

We haven't worked directly on it for a while. The reason is that we are doing a ~large refactoring~ rewrite of our SDK (see https://github.com/realm/realm-js/discussions/5416 for a high-level description). The rewrite will have an impact on Hermes performance, and once we have done the first alpha release of our new implementation, we will evaluate the performance on Hermes (and we invite our users to do the same).

Last week the new implemented landed in our main branch. You are welcome to take a look :smile:

stefoid commented 1 year ago

Good to hear!

Can you also please comment on the general large performance decrease that is proportional to the number of 'live' filtered results sets and the number of records in the database?

Is this expected? Are there best practices to avoid this?

thanks you

RedBeard0531 commented 11 months ago

@stefoid

Can you also please comment on the general large performance decrease that is proportional to the number of 'live' filtered results sets

Yes this is somewhat expected. By 'live', I assume that you mean that you have change notification listeners subscribed to them? Roughly speaking, for every write op, we need to run all subscribed queries to see what has changed, so it is expected that having many of them will be more expensive than having few.

and the number of records in the database?

This is a bit less expected and may be simple to improve. Queries should only be proportional to the data set size if they are not using an index. If they are able to use an index, they should take time (roughly) proportional to the amount of matching data. Do you have any indexes on your data, and do you know if they will help for the queries you are running? If not, please try to add those indexes and see if it helps.

If that doesn't help, or if you need help figuring out which indexes to add, please open a new issue, since this perf issue is unrelated to Hermes, and we'd rather keep that discussion separate.

kraenhansen commented 3 months ago

@liamjones did you manage to complete your migration to Hermes and realm@12? The latest major version of the SDK (v12) is a complete re-write of the SDK and we've taken great care and performed explicit performance profiling in an attempt to avoid performance regressions in the re-write.

As such, I'll close this issue for now - please feel free to create new issues on this when you've upgraded to the latest version of the SDK as it goes without saying, performance is super important for a project like ours 👍

(this also applies to @stefoid - please create a new issue if your issue persists with the latest major version of the SDK).

liamjones commented 3 months ago

@liamjones did you manage to complete your migration to Hermes and realm@12?

Not quite yet but we're making progress towards it! We're now on Hermes-compatible versions of RN, and I need to find enough time to try switching to Hermes. We can't yet move to v12 of Realm from v11 because of this: https://github.com/realm/realm-js/issues/6122 (there's a bunch of tests that'd need to be rewritten to work around it in the interim)

@kraenhansen re performance you may be interested to know that one of the tickets Tom Duncalf was waiting on in the RN project finally landed on main 3 weeks ago (it isn't yet in an RN release): https://github.com/react-native-community/discussions-and-proposals/issues/505#issuecomment-1912168885 I think this relates directly to the performance degradation I originally logged but may no longer be relevant in Realm v12 with the rewritten SDK?

kraenhansen commented 3 months ago

Thanks for sharing the links! I've added #6122 to our backlog of quick wins, I believe it wilk be a simple fix. We'll revisit how we wrap native objects to see if that native state fix can yield extra performance, once it's released 👍