Don't recreate iterator every time #363
Conversation
melvin-bot
bot
requested review from
francoisl
and removed request for
a team
francoisl
left a comment
francoisl
left a comment
There was a problem hiding this comment.
Makes sense, just one small question to clarify
| delete this.storageMap[temp[i]]; | ||
| this.recentKeys.delete(temp[i]); |
There was a problem hiding this comment.
Out of curiosity, why not use drop() here?
There was a problem hiding this comment.
We don't want to remove key from storageKeys as that's used for getAllKeys. We don't remove the key from the db only from cache.
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Makes sense to me, thanks!
lib/OnyxCache.js
Outdated
| removeLeastRecentlyUsedKeys() { | ||
| while (this.recentKeys.size > this.maxRecentKeysSize) { | ||
| const iterator = this.recentKeys.values(); | ||
| let toRemove = this.recentKeys.size - this.maxRecentKeysSize; |
There was a problem hiding this comment.
| let toRemove = this.recentKeys.size - this.maxRecentKeysSize; | |
| let numKeysToRemove = this.recentKeys.size - this.maxRecentKeysSize; |
lib/OnyxCache.js
Outdated
| const temp = []; | ||
| while (toRemove > 0) { | ||
| const value = iterator.next().value; | ||
| if (value !== undefined) { |
There was a problem hiding this comment.
If value is undefined, should we break?
There was a problem hiding this comment.
it shouldn't every be undefined. I will remove it.
| toRemove--; | ||
| } | ||
|
|
||
| for (let i = 0; i < temp.length; ++i) { |
There was a problem hiding this comment.
Why aggregate temp in a separate array? Could we just do these two lines inline instead of in a separate loop?
- temp.push(value);
+ delete this.storageMap[value];
+ this.recentKeys.delete(value);There was a problem hiding this comment.
because it modifies the Set and most likely breaks iterator
Szymon20000
dismissed stale reviews from mountiny and danieldoglas
via
a6af414
5 participants
Turned out that during chat switching we have cases when we remove several items at the same time.
In those cases we don't want to recreate iterator.
Details
Related Issues
GH_LINK
Automated Tests
Linked PRs