[For review only] Minimize rereading of batches in [FSTLocalDocumentsView documentsMatchingCollectionQuery]

[var-const]

Gil, could you please take a quick look to see if this is equivalent to what you had in mind? This is not ready for the full review (I didn't copy some plumbing, it's basically just to verify the idea). This change brings significant performance improvements when querying for a lot of documents, so I'd like to double-check if there is actually a reason to be happy. Integration tests pass.

[wilhuff]

This seems about right, but we can go farther.

[wilhuff]

I'm convinced that at the close of this loop, you can assert that matchingKeys will be empty. Try it!

If so, I believe this whole thing can be simplified as to:

• load the remote documents
• load the mutation batches
• apply the mutation batches to results
• filter results, removing those that don't match

Applying should be like this (sorry for pseudocode):

for batch in batches:
  for mutation in batch:
    if mutation.key is not a direct child of query.collection:
      # keys can't change through mutation, so don't bother tracking
      # documents that can't possibly match
      continue

    mutated = [mutation applyTo:results[mutation.key] documentKey:mutation.key]
    apply mutated to results using the logic currently in -localDocuments:

Note query filtering doesn't happen on the intermediate states, only on the final.

At that point I think you can delete localDocuments and documentsForKeys. localDocument will have just a single caller and can be inlined into documentForKey

[var-const]

Unfortunately, four integration tests trigger the case when matchingKeys are not empty. They all involve writing new documents to a collection while being offline, e.g. testGetCollectionWhileOfflineWithDefaultSource. If I understand the code correctly, remoteCache only contains the documents which were ack'ed by the server, and localDocuments only applies local changes to given documents, which would leave the offline-created doc4 out of results if checking batches was omitted.

I'd be happy to get rid of localDocument somehow, though. Let me know if you can think of a way around the offline issue.

[wilhuff]

What should be happening is that while those documents aren't in the remote documents cache, they are in the mutation queue, so when you iterate through the batches and apply them to results the new documents should be added.

[var-const]

Disregard previous comments, let me try it out...

[wilhuff]

The strategy that localDocuments applies today does not work for what I'm proposing.

It says essentially, for each key already present, apply all mutations for exactly that key to produce the final view of the document.

My proposal in the pseudocode above is to process all mutations that apply to the query such that the mutation will create new documents that don't have a value in remote documents yet.

Note that batches can contain mutations that are in different collections. These batches match the query even though not all the mutations in the batch do. Mutations can't change the key a document though. This is why it's safe to disregard these mutations before the local view has been fully assembled.

[var-const]

Does this look right?

  __block FSTDocumentDictionary *results = [self.remoteDocumentCache documentsMatchingQuery:query];
  NSArray<FSTMutationBatch *> *matchingBatches =
      [self.mutationQueue allMutationBatchesAffectingQuery:query];

  for (FSTMutationBatch *batch in matchingBatches) {
    for (FSTMutation *mutation in batch.mutations) {
      if (mutation.key.path().PopLast() != query.path) {
        // keys can't change through mutation, so don't bother tracking documents that can't
        // possibly match
        continue;
      }

      FSTDocumentKey* key = static_cast<FSTDocumentKey*>(mutation.key);
      FSTMaybeDocument *baseDoc = results[key];
      FSTMaybeDocument *mutatedDoc =
        [mutation applyTo:baseDoc baseDocument:baseDoc localWriteTime:batch.localWriteTime];

      if ([mutatedDoc isKindOfClass:[FSTDeletedDocument class]]) {
        results = [results dictionaryByRemovingObjectForKey:mutation.key];
      } else if ([mutatedDoc isKindOfClass:[FSTDocument class]]) {
        results = [results dictionaryBySettingObject:(FSTDocument *)mutatedDoc forKey:mutation.key];
      } else {
        HARD_FAIL("Unknown document: %s", mutatedDoc);
      }
    }
  }

  // Note that the extra reference here prevents ARC from deallocating the initial unfiltered
  // results while we're enumerating them.
  FSTDocumentDictionary *unfiltered = results;
  [unfiltered
      enumerateKeysAndObjectsUsingBlock:^(FSTDocumentKey *key, FSTDocument *doc, BOOL *stop) {
        if (![query matchesDocument:doc]) {
          results = [results dictionaryByRemovingObjectForKey:key];
        }
      }];

  return results;

(apart from refactoring potential, it actually gives a speed improvement as well)

[wilhuff]

Seems right.

[var-const]

Thanks! This function's execution time is now dominated by dictionaryBySettingObject (called directly and by remote document cache). Perhaps migrating to C++ version of immutable map might improve timings further.

[var-const]

Closing in favor of #1533.