-
Notifications
You must be signed in to change notification settings - Fork 585
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
Performance regression vs non-Hermes version? #4443
Comments
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 |
Thanks Tom! |
@liamjones I'm working on profiling of the Hermes code. Can you share anything about the characteristics of the import you do?
|
Hi @fronck, sorry for the delayed reply, been a busy week! I've cut down a minimal repro and sent it in to [email protected] so you can see the exact code we're running - look for email titled "Private repro for realm-js issues #4443 & #4328" |
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):
|
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. |
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. |
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! |
@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? |
@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 |
@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. |
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. |
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:
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:
For Android:
|
Thanks for doing so though in my case Hermes is not enabled. |
Ah OK, we don't have a workaround for that case yet. I'll update this issue when we have a fix though |
@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 |
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. |
Thanks for doing so. Looking forward to your reply |
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. |
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 I am more curious about the non-Hermes case |
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 |
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? |
@mfbx9da4 The issue is as described here: #4443 (comment) 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 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. |
@liamjones @mfbx9da4 In case it is of interest, I posted an update on the performance regression situation, after a great deal of research, here: #3940 (comment) |
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. |
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? |
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! 😄 |
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. |
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 Ive tried closing some of these live result sets and the performance of the app improves a lot. So the summary is : 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! |
An update from the Realm team on performance improvements is welcome at this stage… |
We haven't worked directly on it for a while. The reason is that we are doing a Last week the new implemented landed in our |
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 |
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.
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. |
@liamjones did you manage to complete your migration to Hermes and 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). |
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: #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): react-native-community/discussions-and-proposals#505 (comment) 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? |
Thanks for sharing the links! I've added #6122 to our backlog of quick wins, I believe it will 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 👍 |
Description
While investigating #4328 (which is now resolved), I noticed that performance seemed to have halved on the Hermes builds when importing several thousand objects (see #4328 (comment)).
Stacktrace & log output
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
The text was updated successfully, but these errors were encountered: