Fix error in Memory LRU implementation #1299
Conversation
gsoltis
requested review from
mikelehen,
rsgowman,
var-const,
wilhuff and
zxu123
as code owners
| @@ -17,7 +17,7 @@ | |||
| import { Equatable } from './misc'; | |||
| import * as objUtil from './obj'; | |||
|
|
|||
| type Entry<K, V> = [K, V]; | |||
| export type Entry<K, V> = [K, V]; | |||
There was a problem hiding this comment.
We already export a type Entry in sorted_map.ts. I would actually prefer if we use that type since it's easier to digest (it's an object on not an array)
There was a problem hiding this comment.
This has to be exported because it's used in the constructor for ObjectMapIterator, it's not actually anywhere in the public api for ObjectMap or the iterator though. I would prefer not to add a dependency from this collection to SortedMap, since they aren't really related.
There was a problem hiding this comment.
If we do keep it, then please rename it. Having two Entry types that are very similar will make auto-importing a bit tricky.
| [Symbol.iterator](): Iterator<{ key: KeyType; value: ValueType }> { | ||
| // We don't care about the keys, all of the actual keys are in the | ||
| // arrays that are the values of the inner object. | ||
| const it = objUtil.values(this.inner)[Symbol.iterator](); |
There was a problem hiding this comment.
This creates a copy of the value array, which is a little surprising for an iterator. I don't know a good way around this without using generators (which I assume we can't), so I am going to ignore this.
I do think this could be simplified quite a bit though. Wouldn't this work just as well:
const values : Array<{ key: KeyType; value: ValueType }> = [];
this.forEach((key, value) => values.push({key, value}));
return values[Symbol.iterator]();
You might not need the ObjectMapIterator.
There was a problem hiding this comment.
Unfortunately, I don't know a way to avoid creating the array of values. Objects don't support iterators by default, and there's no ordering either to be able to start and stop iteration through the object.
As for using forEach, it kinda depends on whether or not we think there are collisions. ObjectMapIterator doesn't need to create all of the entries that will be returned at once. That's only a benefit though if there are collisions, since we do need the values array.
I'm not sure I have a strong opinion either way, since it depends on what's stored. Creating the whole array up front does have the advantage of not needing to export an extra Entry type, I suppose.
Do you feel strongly either way?
There was a problem hiding this comment.
The two solutions (copy every value and copy every set of matching values) have such different complexities that I would much prefer to pick the simpler one.
We can look at the usage in the SDK and see how common collisions are, but from memory it seems that they should be pretty rare (my IDE doesn't open at the moment...). The overhead from an occasional canonical ID collision can probably be ignored.
There was a problem hiding this comment.
Ok, I went with the forEach method.
| /** | ||
| * Implements an Iterator over ObjectMap. | ||
| */ | ||
| export type IteratorEntry<KeyType, ValueType> = { |
There was a problem hiding this comment.
FWIW, This is the Entry type from SortedMap.
There was a problem hiding this comment.
Yup, but I don't think a dependency between them is appropriate.
Iterating orphaned documents was returning all documents, not just orphaned ones. They weren't being incorrectly GC'd, but they were incorrectly counting towards sequence numbers to GC.
The test that caught this is coming in the port of firebase/firebase-ios-sdk#1905
Additionally, I implemented
Symbol.iteratorforObjectMapso that I could usePersistencePromise.forEach().