Keep track of number of queries in the query cache #510

gsoltis · 2018-02-13T21:09:13Z

Adds a targetCount to the global target metadata
Splits QueryCache add and update methods so that we can update the count appropriately.
Adds count() method to QueryCache to report the number of targets currently in the cache
Adds some schema migration infrastructure, partially courtesy of @schmidt-sebastian
Adds a schema migration to ensure that the global target metadata exists, and then updates it with the count of any targets that might already exist in the persistence layer.
Changes LocalStore to call updateQueryData instead of addQueryData in the appropriate place.

iOS counterpart: firebase/firebase-ios-sdk#776

schmidt-sebastian · 2018-02-16T02:44:01Z

packages/firestore/src/local/indexeddb_query_cache.ts

@@ -97,36 +97,76 @@ export class IndexedDbQueryCache implements QueryCache {
    );
  }

+  private saveMetadata(


I was gonna claim ownership of the term 'metadata' (but Mike already shut me down). Can you make this more query-specific (updateQueryMetadata) ?

I think metadata is fine, since it's a private method referring specifically to the instance-scoped field called metadata. I don't think it lays claim to an sdk-wide notion of metadata.

schmidt-sebastian · 2018-02-16T02:46:06Z

packages/firestore/src/local/indexeddb_schema.ts

+ * Schema Version for the Web client:
+ * 1. Initial version including Mutation Queue, Query Cache, and Remote Document
+ *    Cache
+ * 2. Added targetCount to targetGlobal row.


Add note that this is needed for GC?

I'm not sure this is actually a good idea. It's the sort of comment that could easily become outdated by accident, since whoever starts using it for something else would have no reason to check and update this location. I think it's better to stick to just describing what the schema updates are.

schmidt-sebastian · 2018-02-16T02:47:26Z

packages/firestore/src/local/indexeddb_schema.ts

+ *
+ * @param {IDBTransaction} txn The version upgrade transaction for indexeddb
+ */
+function ensureTargetGlobal(


ensureTargetGlobalEntryExists ? The current name raises more questions than it answers :)

Changed to ensureTargetGlobalExists. The comment should also clarify any questions raised.

schmidt-sebastian · 2018-02-16T02:51:28Z

packages/firestore/src/local/simple_db.ts

-        // createObjectStore(), etc. are synchronous.
+        // We are provided a version upgrade transaction from the request, so
+        // we wrap that in a SimpleDbTransaction to allow use of our friendlier
+        // API for schema migration operations.
        const db = (event.target as IDBOpenDBRequest).result;


Can you do the wrapping right here? That way, your comment is immediately related to the next line.

schmidt-sebastian · 2018-02-16T02:53:47Z

packages/firestore/src/local/simple_db.ts

@@ -342,6 +360,14 @@ export class SimpleDbStore<KeyType extends IDBValidKey, ValueType> {
    return wrapRequest<void>(request);
  }

+  // if we ever need more of the count variants, we can add overloads. For now,


Suggestion: /** Counts all objects in the store. Runs in O(n) */

In my opinion, The part about the overloads doesn't seem to warrant a comment.

I left the part about overloads because we are explicitly not supporting part of the underlying API, whereas other methods in this class provide overloads to handle all of the variants of the API calls that they are wrapping. As this is a deviation from what we are currently do, I thought it warranted a comment.

I added the O(n) note though.

schmidt-sebastian · 2018-02-16T02:54:33Z

packages/firestore/test/unit/local/query_cache.test.ts

@@ -47,6 +47,17 @@ describe('MemoryQueryCache', () => {
  });

  genericQueryCacheTests();
+
+  it('cannot remove nonexistent query', async () => {
+    // We don't check on indexeddb query cache, but we do for memory.


I don't understand this comment.

schmidt-sebastian · 2018-02-16T02:55:14Z

packages/firestore/test/unit/local/query_cache.test.ts

+    } catch {
+      gotError = true;
+    }
+    expect(gotError).to.be.true;


So this should only be true for MemoryPersistence?

Yes, I updated the comment.

schmidt-sebastian · 2018-02-16T02:55:26Z

packages/firestore/test/unit/local/query_cache.test.ts

+    // We don't check on indexeddb query cache, but we do for memory.
+    let gotError = false;
+    try {
+      await cache.removeQueryData(testQueryData(QUERY_ROOMS, 1, 1));


You can just fail() here.

gsoltis · 2018-02-16T21:59:16Z

Ok, I think I have addressed everything now.

mikelehen

Sorry for the delay. This basically LGTM but I found a few remaining minor comments / requests, and then this should be good to go.

mikelehen · 2018-02-20T23:00:11Z

packages/firestore/src/local/indexeddb_query_cache.ts

@@ -97,36 +97,79 @@ export class IndexedDbQueryCache implements QueryCache {
    );
  }

+  private saveMetadata(


Move this down with the other private helper methods? (probably next to saveQueryData)

mikelehen · 2018-02-20T23:03:18Z

packages/firestore/src/local/indexeddb_query_cache.ts

    }
+
+    // TODO(gsoltis): add sequence number check


Thanks, I think this is much clearer!

mikelehen · 2018-02-20T23:12:49Z

packages/firestore/src/local/indexeddb_schema.ts

-  db.createObjectStore(DbTargetGlobal.store);
+  let p = PersistencePromise.resolve();
+  if (fromVersion < 2 && toVersion >= 2) {
+    p = p.next(() => ensureTargetGlobal(txn)).next(() => saveTargetCount(txn));


I don't see the rename... I also still don't think there's a need to have two steps here. TargetGlobal was an existing part of the schema in v1. The only thing you added was a new field to it.

Another way to deal with this would be to get rid of ensureTargetGlobal() and have populateTargetCount() just do nothing if the TargetGlobal row doesn't exist yet (since it should get populated correctly as 0 when the TargetGlobal row gets created as a normal part of the client operating). And actually, I think I like this better since 1) it avoids us duplicating the default new DbTargetGlobal( ... ) code.

mikelehen · 2018-02-20T23:14:34Z

packages/firestore/src/local/indexeddb_schema.ts

+ * Counts the number of targets persisted and adds that value to the target
+ * global singleton.
+ *
+ * @param {IDBTransaction} txn The version upgrade transaction for indexeddb


jsdoc type annotation is unnecessary and no longer correct. :-)

mikelehen · 2018-02-20T23:16:59Z

packages/firestore/src/local/indexeddb_schema.ts

+  );
+  const targetStore = txn.store<DbTargetKey, DbTarget>(DbTarget.store);
+  return targetStore.count().next(count => {
+    return globalStore.get(DbTargetGlobal.key).next(metadata => {


metadata is nullable here and so you should be getting an error for not handling it, but unfortunately we are not compiling with strictNullChecks (this was an accident when we migrated from google3 to github; b/73018483 tracks turning it on). In an effort to avoid adding additional debt, can you handle null here?

I fixed it to actually thread the metadata through, so it should not be null anymore.

mikelehen · 2018-02-20T23:20:37Z

packages/firestore/src/local/simple_db.ts


+  constructor(private transaction: IDBTransaction) {


private readonly if you want...

mikelehen · 2018-02-20T23:22:18Z

packages/firestore/src/local/simple_db.ts

+  // all we need is to count everything in a store.
+  //
+  // Returns the number of rows in the store. Execution time, off of the event
+  // loop, is O(# rows).


Use /* ... */ style comment? And I'm not sure what "off of the event loop" is referring to. I would probably drop it unless there's something special about this method compared to other methods on this class...

The execution time is O(n), but the execution occurs on a different thread, so it does not block the event loop. It just means the amount of time until the you get the callback is proportional to the number of items that need to be counted. It was previously requested that I note that it was O(n). I don't feel too strongly either way...

For now I switched the comment style and reworded it slightly.

There are no threads in JavaScript [well, excluding web workers] and everything runs on the event loop. So if you want to say something, you could say, "Execution time is O(# rows), although asynchronous." But since it returns a PersistencePromise, it's probably not worth calling out that it's asynchronous.

Well, yes, there are not multiple threads running js code, but the VM has multiple threads, and it appears that at least in chrome, indexeddb gets its own thread.

Regardless, I dropped the comment about running time. If anyone wants it back, they can send a PR.

mikelehen · 2018-02-20T23:28:36Z

packages/firestore/test/unit/local/query_cache.test.ts

+    // memory. So, this test is specific to the memory-backed query cache.
+    try {
+      await cache.removeQueryData(testQueryData(QUERY_ROOMS, 1, 1));
+      fail('should not have been able to remove nonexistent query');


This fail method just throws an exception, which you'll then swallow... and so I think this test can't fail. You probably want expect.fail() instead, which hooks into the test runner to register the failure rather than relying on an exception.

You could also consider writing it without await, since .then() lets you attach both a success and rejection handler at once:

return cache.removeQueryData(...).then( () => fail(' ... '), (err) => { /* expected. */ });

Alternatively I don't feel strongly that we need tests to cover assertions and since you didn't add tests for updateQueryData() or addQueryData(), I'd be somewhat inclined to just drop this test. If you do that, please undo the QUERY_* / testQueryData() refactor.

Sounds good.

mikelehen · 2018-02-20T23:32:38Z

packages/firestore/test/unit/local/query_cache.test.ts

@@ -68,35 +80,35 @@ describe('IndexedDbQueryCache', () => {
  genericQueryCacheTests();
 });

+const QUERY_ROOMS = Query.atPath(path('rooms'));
+const QUERY_HALLS = Query.atPath(path('halls'));
+const QUERY_GARAGES = Query.atPath(path('garages'));


Now that these are file-level globals can you move them to the top of the file (above persistence and cache)?

Undid this change.

mikelehen · 2018-02-20T23:37:54Z

packages/firestore/test/unit/local/query_cache.test.ts

+    const updated = testQueryData(QUERY_ROOMS, 1, 2);
+    await cache.updateQueryData(updated);
+    const retrieved = await cache.getQueryData(updated.query);
+    expect(retrieved).to.deep.equal(updated);


With this change, setAndReadQuery() is only used once and therefore doesn't justify itself as a helper method. Can you inline the single usage?

gsoltis · 2018-02-21T00:44:07Z

Ok, I can't seem to reply to the comment about not seeing the ensureTargetGlobalExists rename, because github now thinks it's outdated. Anyways, I have renamed it, and it remains necessary because schema 0 does create the object store, but it does not create the row. We need the row to exist before updating the target count, although in that case we are guaranteed that the count is 0. I would rather handle ensuring the row exists in schema migrations. Then, the rest of the code can treat it as guaranteed-to-exist. I've added an assert that it exists in the query cache, since the query cache code gets called after the migration code. This also matches what we do for iOS: https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/Source/Local/FSTLevelDBQueryCache.mm#L96

Otherwise, I think it's weird to do some db initialization in migrations, and some in constructors / start methods.

mikelehen

LGTM with 2 optional nits.

FYI- You can (I think) always reply to a file comment in github by clicking the src/packages/firestore/... link directly above the comment which will take you to the version of the code as-of that comment. It's certainly annoying though.

mikelehen · 2018-02-21T17:01:24Z

packages/firestore/src/local/indexeddb_query_cache.ts

@@ -56,7 +55,8 @@ export class IndexedDbQueryCache implements QueryCache {
  private metadata = new DbTargetGlobal(
    /*highestTargetId=*/ 0,
    /*lastListenSequenceNumber=*/ 0,
-    SnapshotVersion.MIN.toTimestamp()
+    SnapshotVersion.MIN.toTimestamp(),
+    /*targetCount=*/ 0
  );


Since you've reworked this so that the DBTargetGlobal always exists, I think we don't need to initialize metadata here, and I'd prefer not have these defaults duplicated unnecessarily between here and indexeddb_schema.ts. So perhaps change this to just:

private metadata: DbTargetGlobal = null;

Done, and fixed the bug in the tests where we weren't calling start() but didn't know because we were initializing this to a dummy value.

So, good suggestion!

mikelehen · 2018-02-21T17:06:34Z

packages/firestore/src/local/simple_db.ts

+  // all we need is to count everything in a store.
+  //
+  // Returns the number of rows in the store. Execution time, off of the event
+  // loop, is O(# rows).


There are no threads in JavaScript [well, excluding web workers] and everything runs on the event loop. So if you want to say something, you could say, "Execution time is O(# rows), although asynchronous." But since it returns a PersistencePromise, it's probably not worth calling out that it's asynchronous.

gsoltis · 2018-02-23T00:54:44Z

I'm OOO Friday and Monday, so I'm going to wait until Tuesday to merge this, on the off chance that something breaks.

schmidt-sebastian and others added 29 commits February 2, 2018 12:00

Adding Schema Migration

eb44929

Pseudocode for Schema Migration

07e44e7

[AUTOMATED]: Prettier Code Styling

3e1053f

IndexedDb Schema Migration

5e84782

Lint cleanup

fd50301

Removing unused import

be3b463

Removing user ID from instance row

de237c5

[AUTOMATED]: Prettier Code Styling

53e56b5

Review comments

7b3bedb

Merge branch 'master' into multitab-schemamigration

7b97c0a

Lint fixes

c176eff

Review

9154aad

[AUTOMATED]: Prettier Code Styling

6b072ff

Fixing the tests

662365f

Closing the Database in the Schema tests

dd21376

[AUTOMATED]: Prettier Code Styling

5e54b3a

Changing test helper to close the DB

cb81df8

[AUTOMATED]: Prettier Code Styling

7991970

Making v2 the default version

2c6d6e1

[AUTOMATED]: Prettier Code Styling

f16bb4f

Addressing comment

038c159

[AUTOMATED]: Prettier Code Styling

b70303c

Renamed to ALL_STORES

34ab25a

Start work on adding query counts

f45be5a

Merge branch 'master' into add_query_count

44a2da8

Implement target count

fac7d17

[AUTOMATED]: Prettier Code Styling

55f5ff6

Separate out add and update for the query cache

2d955e9

[AUTOMATED]: Prettier Code Styling

d09153a

gsoltis assigned mikelehen Feb 13, 2018

Greg Soltis added 2 commits February 15, 2018 17:51

[AUTOMATED]: Prettier Code Styling

c479800

helpers moved

28f7c48

schmidt-sebastian suggested changes Feb 16, 2018

View reviewed changes

Greg Soltis added 4 commits February 16, 2018 10:44

Review feedback

8e452fa

Switch to just using fail()

edf8376

Rename updateMetadata

6d48526

Renaming and thread metadata through schema upgrade

4a85dfa

gsoltis assigned mikelehen Feb 16, 2018

Greg Soltis added 2 commits February 16, 2018 14:16

Merge branch 'master' into add_query_count

4a74e36

Use the proper assert

cc763dd

mikelehen suggested changes Feb 20, 2018

View reviewed changes

mikelehen assigned gsoltis and unassigned mikelehen Feb 21, 2018

Greg Soltis added 2 commits February 20, 2018 16:44

Review feedback

eb2b0a3

[AUTOMATED]: Prettier Code Styling

67fe837

gsoltis assigned mikelehen and unassigned gsoltis Feb 21, 2018

mikelehen approved these changes Feb 21, 2018

View reviewed changes

Greg Soltis added 2 commits February 22, 2018 15:58

Drop note re running time, initialize metadata to null

860818e

Fix tests that weren't calling start

d9b551f

mikelehen assigned gsoltis and unassigned mikelehen Feb 23, 2018

Greg Soltis added 2 commits February 27, 2018 09:21

Merge branch 'master' into add_query_count

231ef2e

[AUTOMATED]: Prettier Code Styling

097fdd0

gsoltis merged commit 9cda9c9 into master Feb 27, 2018

gsoltis deleted the add_query_count branch February 27, 2018 17:48

firebase locked and limited conversation to collaborators Oct 23, 2019

Keep track of number of queries in the query cache #510

Keep track of number of queries in the query cache #510

Conversation

gsoltis commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsoltis commented Feb 16, 2018

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsoltis commented Feb 21, 2018

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsoltis commented Feb 23, 2018