Firestore: defer copying bloom filter bytes until the bloom filter is actually needed #10981
Conversation
* upload initial code * add golden test with md5 calculator * resolve comments * add static const to kGoldenDocumentPrefix * resolve comments * resolve comments
…/BloomFilter-apply-bloom-filter-on-existence-filter-mismatch
…/BloomFilter-apply-bloom-filter-on-existence-filter-mismatch
… -> if (filter->has_unchanged_names)
…_firestore_v1_BloomFilter_init_zero, for consistency
| bloom_filter_copy->bits.padding = 7; | ||
| bloom_filter_copy->bits.bitmap = | ||
| nanopb::MakeBytesArray(std::vector<uint8_t>{0x42, 0xFE}); | ||
| EXPECT_TRUE(change.filter().bloom_filter().value() == bloom_filter_copy); |
There was a problem hiding this comment.
EXPECT_EQ(change.filter().bloom_filter().value(), bloom_filter_copy); crashes the build.
There was a problem hiding this comment.
The problem was that the operator== for google_firestore_v1_BloomFilter was defined in the wrong place. It was defined in the firebase::firestore::remote namespace in existence_filter.h, but should have been defined in the firebase::firestore namespace (the same namespace in which google_firestore_v1_BloomFilter is defined). Being in the wrong namespace prevented it from being found by ADL (address-dependent lookup) at compile time.
It seemed like a good idea to move the operator== functions out of the existence_filter.h file. So I did that by adding a new operators.h file in the nanopb directory. I also added unit tests for it (which need a bit more work in a follow-up PR).
I also added operator== for nanopb::Message, which was missing. This allows comparing nanopb::Message objects directly with each other, and eventually calling the operator== overloads for the underlying protos.
With these changes, the EXPECT_EQ now works, and the code is much better as well, with proper design and unit test coverage.
See 7c7cb61
… protos: google_firestore_v1_BloomFilter and google_firestore_v1_BitSequence. Move the implementation of the operator== overloads for these two protos from existence_filter.h into the new file operators.h, and into the correct namespace. TODO: Add tests in operators_test.cc for google_firestore_v1_BloomFilter
…y adding `NanopbOperatorsTest_BitSequence` fixture class and a `TestOperatorEquals()` method, which makes each TEST_F() function a single line.
dconeybe
force-pushed
the
mila/BloomFilter
branch
from
3f56c64 to
6435094
Compare
|
I'm abandoning this work, for now. |
2 participants
This PR avoids copying the bytes of the bloom filter sent from the server in the "existence filter" message. Previously, these bytes were copied from the proto into a vector in the "BloomFilterParameters" struct. The bloom filter byte array could be somewhat large, in the ballpark of 8kb, so avoiding the copy is desirable, especially since the bloom filter is only ever used in the somewhat-rare case of an existence filter mismatch.
The challenge of avoiding the copy is that "stealing" the bytes must be done carefully to avoid both memory leaks and double-freeing of the dynamically-allocated byte array. Since the
nanopb::Messageclass models a unique ownership likestd::unique_ptr, we decided to simply pass around thenanopb::Messagefor the bloom filter.See #10953 (comment) for more details.
#no-changelog