Page MenuHomePhabricator

[lib] Add memoization to useKeyserverCall
ClosedPublic

Authored by inka on Sep 12 2023, 4:41 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 11:52 AM
Unknown Object (File)
Wed, Nov 20, 1:22 AM
Unknown Object (File)
Tue, Nov 19, 9:18 PM
Unknown Object (File)
Sun, Oct 27, 10:19 PM
Unknown Object (File)
Sun, Oct 27, 10:19 PM
Unknown Object (File)
Sun, Oct 27, 10:19 PM
Unknown Object (File)
Sun, Oct 27, 10:19 PM
Unknown Object (File)
Sun, Oct 27, 10:19 PM
Subscribers

Details

Summary

issue: https://linear.app/comm/issue/ENG-4889/memoize-action-for-every-keyservercall
Fistly, boundCallServerEndpointSelector is added to memoize the value retuned from bindCookieAndUtilsIntoCallServerEndpoint based on its arguments. bindCookieAndUtilsIntoCallServerEndpoint creates some functions, so it might be beneficial to memoize them. It is also called when the action is being executed - only once the client decides which keyserver they want to call with this action, we can bind the correct cookie and other values to a callServerEndpoint. And we want actions to be as fast as possible.

Secondly, baseCreateBoundKeyserverCallActionSelector/createBoundKeyserverCallActionSelector is added. This is similar to baseCreateBoundServerCallsSelector that was used in useServerCall. _memoize keeps a map of results, keyed by the first argument of the function it memoizes. So what we are doing is creating a selector for each keyserverCall. Every one of those selectors remembers the last value calculated for its action.

Test Plan

Used searchMessagesActionObj described in the previous diff. Checked that searching messages using useKeyserverCall(searchMessagesActionObj); still works.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka edited the test plan for this revision. (Show Details)
inka edited the summary of this revision. (Show Details)
inka requested review of this revision.Sep 12 2023, 4:58 AM

I wonder if it would be possible to rewrite this to use React hooks instead of createSelector. I believe that all uses of createSelector can be replaced by React.useMemo. The selector funcs passed to createSelector turn into dep lists. React hooks are more modern idioms than createSelector, but I'm not sure as to all of the tradeoffs.

lib/utils/keyserver-call.js
58–68 ↗(On Diff #30960)

Nit: shorthand

@ashoat This selector is called inside of actions. I don't think I can use selectors inside of actions, especially that they are called in class components.

inka edited the test plan for this revision. (Show Details)
inka edited the summary of this revision. (Show Details)
inka edited the test plan for this revision. (Show Details)

Clean up - inline getCallKeyserverEndpoint and invert memoization

We already pass all actions to a hook (even in class components, we use useServerCall) so I imagine there is some way to do it. But I won't insist

Now the memoization works in a slightly different way, but achieves the same effect - memoizes the function.
We create a selector, that returns a _memoized function. This function remembers the last action returned for every KeyserverCall. This selector remembers the _memoized function, so it can be used in all components calling useKeyserverCall. If this selector's dependencies change, all actions would have to be recalculated anyway, so it fine that the memoized function is created anew.
This way, we have to memoize less things, because we memoize one function, and actions for all ActionFuncs, instead of a selector and an action for every ActionFunc

We already pass all actions to a hook (even in class components, we use useServerCall) so I imagine there is some way to do it. But I won't insist

boundCallServerEndpointSelector is called inside of callKeyserverEndpoint, which is being called inside of the function returned by ActionFunc. This would be equivalent to being able to use a selector inside of callServerEndpoint, which I don't think is possible.
Sorry, I think I called an action the wrong thing maybe.

lib/utils/keyserver-call.js
115 ↗(On Diff #31164)

We should probably memoize it further, per keserverID

Memoize boundCallServerEndpointSelector per keserverID

michal added inline comments.
lib/utils/keyserver-call.js
80 ↗(On Diff #31208)

nit

This revision is now accepted and ready to land.Sep 20 2023, 3:03 AM
In D9143#270092, @inka wrote:

We already pass all actions to a hook (even in class components, we use useServerCall) so I imagine there is some way to do it. But I won't insist

boundCallServerEndpointSelector is called inside of callKeyserverEndpoint, which is being called inside of the function returned by ActionFunc. This would be equivalent to being able to use a selector inside of callServerEndpoint, which I don't think is possible.
Sorry, I think I called an action the wrong thing maybe.

Is this still "not possible" (I'm sure it was always possible with enough work) now that you've migrated to defining actions in hooks?

In D9143#270092, @inka wrote:

We already pass all actions to a hook (even in class components, we use useServerCall) so I imagine there is some way to do it. But I won't insist

boundCallServerEndpointSelector is called inside of callKeyserverEndpoint, which is being called inside of the function returned by ActionFunc. This would be equivalent to being able to use a selector inside of callServerEndpoint, which I don't think is possible.
Sorry, I think I called an action the wrong thing maybe.

Is this still "not possible" (I'm sure it was always possible with enough work) now that you've migrated to defining actions in hooks?

If I changed bindCallKeyserverEndpointSelector to be a React.useMemo, then this memo would be inside of the useKeyserverCall. useKeyserverCall is a hook, so in every component we create a new instance of it, so the React.useMemo would be calculated for every component separately. This would defeat the purpose of this memoization. We want to have only one instance of this function that the bindCallKeyserverEndpointSelector returns.
As for createBoundServerCallsSelector - I still don't think it's possible to turn it into a useMemo, because it is being called from within the action (here by action I mean the function returned by ActionFuncs ). The actions are returned by hooks, but are not hooks themselves, so they cannot call hooks

Maybe this memoization could be done in some completely different way, but I don't think it's worth doing now. This is my September / October goal. I don't think it's worth spending more time on.

That makes sense, thanks for explaining. I think an important thing to note here is that we would need to introduce a new Provider here to handle caching globally (rather than per-component) in order to have parity with the old createSelector approach

This revision was automatically updated to reflect the committed changes.