Repository navigation
[HOLD for payment 2024-07-17] LOW: [HOLD] Create secondary maps for connection keys in Onyx #35809
Description
Coming from here
Problem
On the connect from Onyx, we store in a map (callbackToStateMapping) a unique connectionID with a mapping of the connected component (which has all the information about the component and configs), in that mapping there is a key which is the used in the withOnyx or the raw Onyx.connext to listen for value changes.
When a value for a key changes, the function keyChanged or keysChanged (if it is a collection) is being called, passing the key and some data. What that function does is, get all the connection IDs from the callbackToStateMapping, and goes through all the items checking if that connection has the key that changed. This could be a really low process, because it look into all the keys connected in the app, and as we can see in the screenshot, a really small account with a few chats and messages can create hundred of connections, and on top of that, the keyChanged function can be called multiple times.
Solution
Create a secondary map, which is the Key that is being listen (like session) and an array of connection IDs that uses that key. This will improve the look up and sending the changes because it look for the connections that are being listening to that Key.
// on connect function
+ if (!onyxKeyToConnectionIDs.has(mapping.key)) {
+ onyxKeyToConnectionIDs.set(mapping.key, []);
+ }
+ onyxKeyToConnectionIDs.get(mapping.key).push(connectionID);
// on keyChanged/keysChanged function
- const stateMappingKeys = _.keys(callbackToStateMapping);
+ const stateMappingKeys = onyxKeyToConnectionIDs.get(key);
// on disconnect function
+ if (onyxKeyToConnectionIDs.has(keyToDelete)) {
+ onyxKeyToConnectionIDs.set(keyToDelete, _.without(onyxKeyToConnectionIDs.get(keyToDelete), connectionID));
+ }
Running a performance test on the keyChanged function we can notice a big improvement: onyx notify connections [function]: 13.7 ms → 1.5 ms (-12.2 ms, -88.9%) 🟢🟢 | 1 → 1
(this test created around 10k of keys and connections)
We did some tests on Android Production build and we got some results:
Report Load:
Hot load:
without patch -> 3677 ms
with patch -> 3380 ms
Improvement -> 300ms
Cold load:
without patch -> 34936 ms
with patch -> 4756 ms
Improvement -> 30180 ms
Metadata
Metadata
Labels
Type
Projects
- Status
Activity
melvin-bot commentedon Feb 5, 2024
Triggered auto assignment to @anmurali (
NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.gedu commentedon Feb 5, 2024
I am from Callstack - expert contributor I would like to work on this
gedu commentedon Feb 7, 2024
@mountiny I created a PR, which is a the app with an Onyx patch with my proposed changes, so they can be tested using the app. if all goes good, I can implement those changes into Onyx and make a new PR
36 remaining items
melvin-bot commentedon Jul 10, 2024
BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
[-][HOLD for payment 2024-07-17] LOW: [HOLD] Create secondary maps for connection keys in Onyx[/-][+][HOLD for payment 2024-07-22] [HOLD for payment 2024-07-17] LOW: [HOLD] Create secondary maps for connection keys in Onyx[/+]melvin-bot commentedon Jul 15, 2024
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.6-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-07-22. 🎊
For reference, here are some details about the assignees on this issue:
melvin-bot commentedon Jul 15, 2024
BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
[-][HOLD for payment 2024-07-22] [HOLD for payment 2024-07-17] LOW: [HOLD] Create secondary maps for connection keys in Onyx[/-][+][HOLD for payment 2024-07-17] LOW: [HOLD] Create secondary maps for connection keys in Onyx[/+]johncschuster commentedon Jul 16, 2024
@gedu, do we need a regression test here? If so, can you propose the steps?
johncschuster commentedon Jul 17, 2024
No payment required here. We're just waiting on the regression test steps.
melvin-bot commentedon Jul 17, 2024
Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.
johncschuster commentedon Jul 18, 2024
Bumped @gedu in Slack about the regression test suite
gedu commentedon Jul 18, 2024
I don't think a regression test is needed