Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

keyChange update key/connectionID for faster lookup #520

Closed
wants to merge 13 commits into from
7 changes: 7 additions & 0 deletions lib/Onyx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,11 @@ function connect<TKey extends OnyxKey>(connectOptions: ConnectOptions<TKey>): nu
callbackToStateMapping[connectionID] = mapping as Mapping<OnyxKey>;
callbackToStateMapping[connectionID].connectionID = connectionID;

// When keyChanged is called, the key is passed and looks through all the callbackToStateMapping the right key
// to avoid having to loop through all the keys all the time (even when just one connection belongs to one key),
// we store the key by connectionID, to access the specific list of connectionIDs
mountiny marked this conversation as resolved.
Show resolved Hide resolved
OnyxUtils.storeKeyByConnections(mapping.key, callbackToStateMapping[connectionID].connectionID);
mountiny marked this conversation as resolved.
Show resolved Hide resolved

if (mapping.initWithStoredValues === false) {
return connectionID;
}
Expand Down Expand Up @@ -199,6 +204,8 @@ function disconnect(connectionID: number, keyToRemoveFromEvictionBlocklist?: Ony
OnyxUtils.removeFromEvictionBlockList(keyToRemoveFromEvictionBlocklist, connectionID);
}

OnyxUtils.deleteKeyByConnections(lastConnectionID);

delete callbackToStateMapping[connectionID];
}

Expand Down
65 changes: 63 additions & 2 deletions lib/OnyxUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@
// Keeps a copy of the values of the onyx collection keys as a map for faster lookups
let onyxCollectionKeyMap = new Map<OnyxKey, OnyxValue<OnyxKey>>();

// Holds a mapping of the connected key to the connectionID for faster lookups
const onyxKeyToConnectionIDs = new Map();

// Holds a list of keys that have been directly subscribed to or recently modified from least to most recent
let recentlyAccessedKeys: OnyxKey[] = [];

Expand Down Expand Up @@ -295,6 +298,32 @@
return evictionAllowList.some((key) => isKeyMatch(key, testKey));
}

/**
* Stores a connection ID associated with a given key.
*
* @param connectionID - a connection ID of the subscriber
* @param key - a key that the subscriber is connected to
*/
function storeKeyByConnections(key: OnyxKey, connectionID: number) {
if (!onyxKeyToConnectionIDs.has(key)) {
onyxKeyToConnectionIDs.set(key, []);
}
onyxKeyToConnectionIDs.get(key).push(connectionID);
}

/**
* Deletes a connection ID associated with its corresponding key.
*
* @param {number} connectionID - The connection ID to be deleted.
*/
function deleteKeyByConnections(connectionID: number) {
const subscriber = callbackToStateMapping[connectionID];

if (subscriber && onyxKeyToConnectionIDs.has(subscriber.key)) {
onyxKeyToConnectionIDs.set(subscriber.key, _.without(onyxKeyToConnectionIDs.get(subscriber.key), connectionID));

Check failure on line 323 in lib/OnyxUtils.ts

View workflow job for this annotation

GitHub Actions / lint

'_' refers to a UMD global, but the current file is a module. Consider adding an import instead.
gedu marked this conversation as resolved.
Show resolved Hide resolved
}
}

/**
* Tries to get a value from the cache. If the value is not present in cache it will return the default value or undefined.
* If the requested key is a collection, it will return an object with all the collection members.
Expand Down Expand Up @@ -381,6 +410,24 @@

evictionBlocklist[key].push(connectionID);
}
/**
* It extracts the pure, non-numeric part of a given key.
*
* For example:
* - `getPureKey("report_123")` would return "report_"
* - `getPureKey("report")` would return "report"
* - `getPureKey("report_")` would return "report_"
* - `getPureKey(null)` would return ""
*
* @param {OnyxKey} key - The key to process.
* @return {string} The pure key without any numeric
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JSDoc still needs to be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrispader what needs to be updated? can you help me I don't recall

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the name and maybe we can change the description to

It extracts the non-numeric collection identifier of a given key. or sth.

getCollectionKey instead of getPureKey

function getPureKey(key: OnyxKey): string {
mountiny marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@chrispader chrispader May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the name of this function a bit misleading. What is a pure key? Why is the "collection part" pure?

Shouldn't we call this sth like: getCollectionKeyFromKey?

Also, think we should not just remove the non-numeric part to get the key part that's defining the collection.

Technically, it would be possible to have a collection called something like 101products_ or report5_

Instead we should scrap the "collection part" based on the valid/allowed collection keys configured here:

const collectionValues = Object.values(keys.COLLECTION ?? {});
onyxCollectionKeyMap = collectionValues.reduce((acc, val) => {
acc.set(val, true);
return acc;
}, new Map());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like those suggestions, that would be more futureproof

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrispader I can make that change, that name I just keep it when I was doing some proof of concepts, I can change it.
I also just change it, to add more support to cases like "report_add", "report_test1". That function is to clean, and get the main key, from "report_123" get "report_", using the key from keys.COLLECTION I would have to loop through the collection keys, and check the startsWith.

This comment was marked as outdated.

if (!key) {
return '';
}
return key.replace(/\d+/g, '');
}
mountiny marked this conversation as resolved.
Show resolved Hide resolved

/**
* Take all the keys that are safe to evict and add them to
Expand Down Expand Up @@ -442,6 +489,7 @@
// individual collection key member for the collection that is being updated. It is important to note that the collection parameter cane be a PARTIAL collection
// and does not represent all of the combined keys and values for a collection key. It is just the "new" data that was merged in via mergeCollection().
const stateMappingKeys = Object.keys(callbackToStateMapping);

for (let i = 0; i < stateMappingKeys.length; i++) {
const subscriber = callbackToStateMapping[stateMappingKeys[i]];
if (!subscriber) {
Expand Down Expand Up @@ -626,10 +674,21 @@
removeLastAccessedKey(key);
}

// We are iterating over all subscribers to see if they are interested in the key that has just changed. If the subscriber's key is a collection key then we will
// We get the subscribers interested in the key that has just changed. If the subscriber's key is a collection key then we will
// notify them if the key that changed is a collection member. Or if it is a regular key notify them when there is an exact match. Depending on whether the subscriber
// was connected via withOnyx we will call setState() directly on the withOnyx instance. If it is a regular connection we will pass the data to the provided callback.
const stateMappingKeys = Object.keys(callbackToStateMapping);
// Given the amount of times this function is called we need to make sure we are not iterating over all subscribers every time. On the other hand, we don't need to
// do the same in keysChanged, because we only call that function when a collection key changes, and it doesn't happen that often.
let stateMappingKeys = onyxKeyToConnectionIDs.get(key);

if (!stateMappingKeys) {
// Getting the collection key from the specific key because only collection keys were stored in the mapping.
stateMappingKeys = onyxKeyToConnectionIDs.get(getPureKey(key));
if (!stateMappingKeys) {
return;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also NAB and just code style discussion / how explicit we want to be.

It might be more explicit:

const connectionKey = isCollectionKey(key) ? getPureKey(key) : key;
const stateMappingKeys = onyxKeyToConnectionIDs.get(connectionKey)
if (!stateMappingKeys) {
  return;
}

Here we explicitly check for a collection key, instead of implicitly doing that by having a fallback check, which is more verbose

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the time the key exists and isn't a collection, also when it enters the main loop

for (let i = 0; i < stateMappingKeys.length; i++) {

it also checks for isCollectionKey, wanna avoid extra computation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, makes sense. If its also that way for performance reasons, might be worth to add a comment to avoid future regressions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something that I'm just realize, is when sending a key that is report_1234 it sends to any component listening to that specific key, but it also sends updates to all components listening to report_, I'm looking into a way to update my code + keep the performance

This comment was marked as outdated.

for (let i = 0; i < stateMappingKeys.length; i++) {
const subscriber = callbackToStateMapping[stateMappingKeys[i]];
if (!subscriber || !isKeyMatch(subscriber.key, key) || !canUpdateSubscriber(subscriber)) {
Expand Down Expand Up @@ -1108,6 +1167,8 @@
prepareKeyValuePairsForStorage,
applyMerge,
initializeWithDefaultKeyStates,
storeKeyByConnections,
deleteKeyByConnections,
};

export default OnyxUtils;
Loading