Use ObjectMap instead of Map with proper comparator. #6018
Conversation
|
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1This report is too large (836,786 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.Test Logs |
ehsannas
force-pushed
the
ehsann/use-objmap-for-overlays
branch
from
3d54caf to
960ce1e
Compare
| import { DocumentKeySet } from '../model/collections'; | ||
| import { | ||
| DocumentKeySet, | ||
| DocumentKeyToMutationMap, |
There was a problem hiding this comment.
I am not a huge fan of the names... could these just be MutationMap and OverlayMap? Less precise but a little more compact. Feel free to push back.
There was a problem hiding this comment.
that sounds fine. The one thing that I'm thinking about is that there are these existing type definitions:
export type DocumentMap = SortedMap<DocumentKey, Document>;
export type MutableDocumentMap = SortedMap<DocumentKey, MutableDocument>;and the new ones I've added are:
export type OverlayMap = ObjectMap<DocumentKey, Overlay>;
export type MutationMap = ObjectMap<DocumentKey, Mutation>;I wonder if I should use SortedMap to be consistent with those two existing types.
e.g.
for a DocumentMap, one should do documentMap = documentMap.insert(...)
for a MutationMap, one should do mutationMap.set(...) (it's a void function).
Having said that, the key->overlay map and key->mutation map don't need to be sorted, so using a SortedMap for them would waste some cpu cycles to keep them sorted for no reason. I'd guess that SortedMap's performance is non-negligibly slower than ObjectMap as it also has to return a new copy of the map every time there's an addition/deletion.
Was there a specific reason that we chose to use SortedMap for DocumentMap and MutableDocumentMap ? Do they really need to stay sorted?
(also Ideally they should have been called SortedDocumentMap and SortedMutableDocumentMap).
There was a problem hiding this comment.
The sorted maps are actually pretty expensive - they are not only immutable and create new copies, but they are backed by a red/black tree so most operations are O(log n). I would steer away from them unless needed. I do think that this advice also applies to most existing usages of DocumentMap and MutableDocumentMap - and they probably do not need to be sorted in most places (but they do in some - e.g. code similar to the one here https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/local/indexeddb_remote_document_cache.ts#L232).
So what this means is that I think we should go with the more concise names OverlayMap and MutationMap and then separately audit the use of DocumentMap and MutableDocumentMap/
There was a problem hiding this comment.
Sounds good. I'll file an issue to audit uses of DocumentMap and MutableDocumentMap.
| @@ -110,4 +110,8 @@ export class ObjectMap<KeyType, ValueType> { | |||
| isEmpty(): boolean { | |||
| return isEmpty(this.inner); | |||
| } | |||
|
|
|||
| size(): number { | |||
| return objectSize(this.inner); | |||
There was a problem hiding this comment.
This is pretty expensive and will turn into an O(n) scan for every Overlay that we return. Can we track the size manually?
Changeset File Check ✅
|
|
@schmidt-sebastian PTAL |
| @@ -47,6 +50,22 @@ export function documentMap(): DocumentMap { | |||
| return EMPTY_DOCUMENT_MAP; | |||
| } | |||
|
|
|||
| export type OverlayMap = ObjectMap<DocumentKey, Overlay>; | |||
| export function newDocumentKeyToOverlayMap(): OverlayMap { | |||
| } | ||
|
|
||
| export type MutationMap = ObjectMap<DocumentKey, Mutation>; | ||
| export function newDocumentKeyToMutationMap(): MutationMap { |
I used
MapandSetin PR #5969, but these data structures use reference comparison for the keys instead of using the object's comparator function. This PR fixes this by usingObjectMapandSortedSet.