[lib] Introduce useDerivedObject for caching whole object when no values change
ClosedPublic
Actions

Authored by ashoat on Dec 27 2023, 6:35 AM.

Details

Reviewers

inka
rohan

Commits

rCOMM61e99eed8084: [lib] Introduce useDerivedObject for caching whole object when no values change

Summary

We only need a subset of a KeyserverInfoPartial for constructing a keyserver call. Let's call that subset KeyserverCallInfo.

We can introduce a selector that will cache a KeyserverCallInfo for a given KeyserverInfoPartial. But we still have a problem, because we still need a way to make sure the whole object of KeyserverCallInfos doesn't change if a single KeyserverCallInfo changes.

This useDerivedObject does that. However, we still have an issue owing to the fact that the useDerivedObject will cache on a per-hook-invocation level, whereas bindCallKeyserverEndpointSelector is caching globally. The following diff will resolve that.

Depends on D10464

Test Plan

Before this stack, I was able to reproduce ENG-3612 by going to the ThreadSettings screen in native while my local keyserver was down. After this stack, the issue no longer repros.

I also compiled a release build of the iOS app to my phone to confirm that there were no regressions in TTI, or the time it takes to open a MessageList and go back to the ChatThreadList.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ashoat created this revision.Dec 27 2023, 6:35 AM

Herald added a subscriber: tomek. · View Herald TranscriptDec 27 2023, 6:35 AM

ashoat added a child revision: D10466: [lib] Separate out bindKeyserverCall cache for each useKeyserverCall callsite.Dec 27 2023, 6:36 AM

ashoat edited the test plan for this revision. (Show Details)Dec 27 2023, 6:48 AM

Harbormaster completed remote builds in B25348: Diff 35018.Dec 27 2023, 7:03 AM

ashoat requested review of this revision.Dec 27 2023, 7:03 AM

But we still have a problem, because we still need a way to make sure the whole object of KeyserverInfoPartials doesn't change if a single KeyserverCallInfo changes. This useDerivedObject does that.

I don't understand what you mean by this. Do you mean that we don't want to change keyserverCallInfos if a single KeyserverCallInfo changes? But if one KeyserverCallInfo changes, then changeOccurred is set to true and keyserverCallInfos is changed by useDerivedObject

Or did you mean that we don't want to change keyserverCallInfos if a single KeyserverInfoPartial changes [but its derived KeyserverCallInfo doesn't]?

No. I'm saying that if NONE of the keyserverCallInfos changes, we don't want to construct a new object that contains the exact same set of keyserverCallInfos.

Or did you mean that we don't want to change keyserverCallInfos if a single KeyserverInfoPartial changes [but its derived KeyserverCallInfo doesn't]?

I guess this is sort of what I'm saying, but a little too specific.

If keyserverCallInfos changes in some way that DOES NOT result in ANY KeyserverInfoPartial changing, we need a way to make sure the whole object of KeyserverInfoPartials doesn't change either. That's the hard problem here.

ashoat mentioned this in D10466: [lib] Separate out bindKeyserverCall cache for each useKeyserverCall callsite.Dec 28 2023, 8:27 AM

If keyserverCallInfos changes in some way that DOES NOT result in ANY KeyserverInfoPartial changing, we need a way to make sure the whole object of KeyserverInfoPartials doesn't change either. That's the hard problem here.

keyserverCallInfos is derived from KeyserverInfoPartials. KeyserverInfoPartials come from state.keyserverStore.keyserverInfos and paramOverride, they don't depend on anything in this code.

Do you mean that if keyserverInfoPartials changes in some way that does not result in any keyserverCallInfo changing, we don't want to create a new keyserverCallInfos object? As you said here:

No. I'm saying that if NONE of the keyserverCallInfos changes, we don't want to construct a new object that contains the exact same set of keyserverCallInfos.

I think I understand what you mean, and I understand the code, and this is exactly what it does - creates keyserverCallInfos based on keyserverInfoPartials, and caches keyserverCallInfos with useDerivedObject, so that even if keyserverInfoPartials changes, but all calculated keyserverCallInfos are the same, the keyserverCallInfos object is not changed - the old reference is returned. I think this code is correct, we are just having trouble communicating. I think that in some cases you write KeyserverInfoPartial when you mean keyserverCallInfo, and vice versa. Please tell me if I'm still misunderstanding.

Oops, yes you’re right – in my most recent comment, when I said KeyserverInfoPartial, I meant KeyserverCallInfo. I can see also that I made the same mixup in the diff description…

ashoat edited the summary of this revision. (Show Details)Dec 29 2023, 5:07 AM

inka accepted this revision.Dec 29 2023, 5:22 AM

This revision is now accepted and ready to land.Dec 29 2023, 5:22 AM

Closed by commit rCOMM61e99eed8084: [lib] Introduce useDerivedObject for caching whole object when no values change (authored by ashoat). · Explain WhyDec 29 2023, 7:28 AM

This revision was automatically updated to reflect the committed changes.

ashoat added a commit: rCOMM61e99eed8084: [lib] Introduce useDerivedObject for caching whole object when no values change.

ashoat mentioned this in D11656: [lib] Use useDerivedObject to reduce renders due to preRequestUserState.Fri, Apr 12, 2:15 PM

ashoat mentioned this in rCOMM7afb2d14a6a8: [lib] Use useDerivedObject to reduce renders due to preRequestUserState.Mon, Apr 15, 9:40 AM