Page MenuHomePhabricator

[native] Use threadInfo in setClientDBStoreActionType
AbandonedPublic

Authored by michal on Sep 21 2023, 4:27 AM.
Tags
None
Referenced Files
F3388621: D9255.id31334.diff
Fri, Nov 29, 3:14 PM
Unknown Object (File)
Tue, Nov 26, 3:52 AM
Unknown Object (File)
Mon, Nov 11, 6:55 PM
Unknown Object (File)
Thu, Oct 31, 1:04 PM
Unknown Object (File)
Thu, Oct 31, 1:04 PM
Unknown Object (File)
Oct 1 2024, 4:03 PM
Unknown Object (File)
Oct 1 2024, 3:58 PM
Unknown Object (File)
Sep 10 2024, 12:07 PM
Subscribers

Details

Summary

Part of ENG-4959

The hashes will be kept in the threadStore alongside the threadInfos, but will be persisted in the redux persist. Because of that we need to change the action to only contain threadInfos and not the whole threadStore.

Explanation why the hashes should be stored here:

  • we need to store the hashes because generating hashes for all threads would take ~20s, so generating them at each app startup is not feasible
  • we could store them in RawThreadInfo but that would mean much larger changes in the codebase. It also adds some complexity (we generate hash from RawThreadInfo but the hash is contained inside of it?)
  • we could store it in some other hashStore, but that would mean we need larger changes in thread-store-ops.js (where we will keep the hashes up to date with the current threadInfos)
Test Plan

Open the app as a logged in user, check that the threads are still there. In redux devtools check that the action was sent correctly

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ashoat added a subscriber: atul.

I’d like to make sure @atul takes a look from the performance angle. Including this in ThreadStore is risky... any changes to the hashes will force every selector that depends on ThreadStore to re-run. If everything is memoized correctly, the performance cost should be limited. But if not there's a risk of hurting client performance by forcing unnecessary renders, especially if the actions "trickle in" separate from other changes to ThreadStore. (I assume "trickling in" would at least be necessary for a fresh log-in, where calculating all the hashes right away would be prohibitively expensive.)

It would be good if @michal could share how the new parts of ThreadStore will get updated. In particular: in what cases will updates will performed alongside changes to the ThreadInfos, versus updates being performed separately at a later point?

If we do go this route, I think we'll need some performance testing (React profiling of before / after) in the final Test Plan in the stack to make sure there is no regression.

we could store it in some other hashStore, but that would mean we need larger changes in thread-store-ops.js (where we will keep the hashes up to date with the current threadInfos)

This would be a much safer approach in my opinion. I might call that something like integrityStore. We can make sure its reducer runs after the thread reducer, which would allow us to still utilize thread store ops to generate changes.

Separately: won’t some selectors need to be updated alongside this change? I'm surprised Flow isn't complaining.

Selector performance:
From looking at the code, we always select by state.threadStore.threadInfos, so there should be no issue. I've put up the next diff (D9256) which contains the actual changes to the threadStore: the newly added threadHashes are always updated at the same time as threadInfos inside of the threadStoreOpsHandlers.

Migration and log in performance:
In the next diffs I'm changing the approach to calculating the "whole store hash" from hashing the stringified store, to calculating hashes for each info separately and then combining them which is faster. On emulator hashing of one threadInfo takes about 8-10ms which I assumed would be fast enough. But I've thought about it more and I'm probably going to additionally change the state check so that the id conversion runs on the keyserver instead of the client because:

  • hashing without conversion takes <1ms
  • then we would have all id conversion on the keyserver which would simplify possible keyserver db migration (if want to stop converting ids between server and client in the future)
  • it's easier to scale the server

integrityStore:
Thanks for the idea about using the thread ops later in the reducer, I didn't thought about that, and assumed I needed to modify threadStoreOpsHandlers. Using integrityStore would also simplify the persist config. Regardless of performance considerations I will experiment with this approach.

Thanks @michal, that all makes sense.

If the performance impact on log-in is low, we can consider calculating the hashes at the same time the threadInfos get added to Redux. But if we take that approach, I'll like to understand the performance impact (eg. TTL, TTI) on login before / after the change.