Page MenuHomePhabricator

[lib] Add memoization to useKeyserverCall
AcceptedPublic

Authored by inka on Tue, Sep 12, 4:41 AM.

Details

Reviewers
michal
kamil
ginsu
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
No Lint Coverage
Unit
No Test Coverage

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.Tue, Sep 12, 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

nit

This revision is now accepted and ready to land.Wed, Sep 20, 3:03 AM