Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c9b8a5
try
var-const Jul 12, 2018
b26dfc7
fix formatting
var-const Jul 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 8 additions & 14 deletions Firestore/Source/Local/FSTLocalDocumentsView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,17 +111,13 @@ - (FSTDocumentDictionary *)documentsMatchingDocumentQuery:(const ResourcePath &)

- (FSTDocumentDictionary *)documentsMatchingCollectionQuery:(FSTQuery *)query {
// Query the remote documents and overlay mutations.
// TODO(mikelehen): There may be significant overlap between the mutations affecting these
// remote documents and the allMutationBatchesAffectingQuery mutations. Consider optimizing.
__block FSTDocumentDictionary *results = [self.remoteDocumentCache documentsMatchingQuery:query];
results = [self localDocuments:results];
NSArray<FSTMutationBatch *> *matchingBatches =
[self.mutationQueue allMutationBatchesAffectingQuery:query];
results = [self localDocuments:results inBatches:matchingBatches];

// Now use the mutation queue to discover any other documents that may match the query after
// applying mutations.
DocumentKeySet matchingKeys;
NSArray<FSTMutationBatch *> *matchingMutationBatches =
[self.mutationQueue allMutationBatchesAffectingQuery:query];
for (FSTMutationBatch *batch in matchingMutationBatches) {
for (FSTMutationBatch *batch in matchingBatches) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for (FSTMutation *mutation in batch.mutations) {
// TODO(mikelehen): PERF: Check if this mutation actually affects the query to reduce work.

Expand All @@ -133,25 +129,23 @@ - (FSTDocumentDictionary *)documentsMatchingCollectionQuery:(FSTQuery *)query {
}

// Now add in results for the matchingKeys.
FSTMaybeDocumentDictionary *matchingKeysDocs = [self documentsForKeys:matchingKeys];
FSTMaybeDocumentDictionary *matchingKeysDocs =
[self documentsForKeys:matchingKeys inBatches:matchingBatches];
[matchingKeysDocs
enumerateKeysAndObjectsUsingBlock:^(FSTDocumentKey *key, FSTMaybeDocument *doc, BOOL *stop) {
if ([doc isKindOfClass:[FSTDocument class]]) {
results = [results dictionaryBySettingObject:(FSTDocument *)doc forKey:key];
}
}];

// Finally, filter out any documents that don't actually match the query. Note that the extra
// reference here prevents ARC from deallocating the initial unfiltered results while we're
// enumerating them.
// 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;
}

Expand Down