ephemeraHQ / converse-app

https://converse.xyz
MIT License
47 stars 5 forks source link

Disable structural sharing #669

Closed nmalzieu closed 2 months ago

nmalzieu commented 2 months ago

Kept digging on the app that freezes sometimes From my latest PR : https://github.com/ephemeraHQ/converse-app/pull/656 I knew that the MMKV storage (for react-query) had smth to do with it. This has been confirmed in DEV, enabling / disabling the persistence for react-query causes / removes the freeze.

This PR disables structuralSharing From the react-query docs, structuralSharing aims to improve perf, but

If you are seeing performance issues because of large responses for example, you can disable this feature with the config.structuralSharing flag

I can confirm that on my failing dev env, this fixes the issue. However I don't know for how long!

Thoughts, cc @alexrisch

alexrisch commented 2 months ago

@nmalzieu Good find on this

Definitely not ideal to have every query key updating the entire cache, I'll try to see if there's something else that can be done there, but also likely will still be a longer term concern with memory

This should probably be a sign to remove some of our current usage of mmkv (based on the discussion in the thread you linked)

Do we feel comfortable sticking with the current SQL implementation or want to change to WatermelonDb? WatermelonDB has documentation for use in Shared Containers: https://watermelondb.dev/docs/Advanced/SharingDatabaseAcrossTargets

Pros of Current SQL: It is already implemented

Cons: There's some concern actually using it in the shared containers Not Reactive without custom implementation

Pros of WatermelonDB: Can be reactive

Cons: Unknown Unknowns Not already implemented

nmalzieu commented 2 months ago

@alexrisch

This should probably be a sign to remove some of our current usage of mmkv

Apart from react-query we don't have heavy usage, most of the data is in sqlite. I would say the biggest storage might be profiles (and it might get big in the long run!).

watermelondb documentation to "use in shared container" is just how to store it in the right folder (the shared container). That's what we do with current sql. Nothing really new and I don't think it will change anything about our crashes! The reactive part might be interesting though if we want to make it our main storage throughout the app.

I would say we go for watermelon if we really really want a reactive sqlite storage, apart from that I don't see what it brings - we could test it if it's not a lot of work of course.

alexrisch commented 2 months ago

I imagine the memory is specific to JSON.stringify and JSON.parse which are known to not be very performant with hermes (at least last I saw) that is being done on every query update

But don't you think we can run into performance issues long term loading every conversation and message into memory to start the app?

nmalzieu commented 2 months ago

Yes probably needs some more optimization! However we only load 50 messages from each convo during initial hydration https://github.com/ephemeraHQ/converse-app/blob/main/data/index.ts#L45 But still, at some point we'll need some optimization. We are never sure of what will break first in terms of perf. What's really clear is that the mmkv persistence of react query serializing on every query is absolutely killing the JS thread!