Merging the LRU branch #1600
Merged
Merging the LRU branch #1600
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 6 commits
July 18, 2018 12:33
* Bump sequence number on resume token refresh * Style * Fix comment formatting * Add FSTReferenceDelegate definition and documentation * Add methods to return nil for delegates, wire up inMemoryPins * Add hook for removing a reference * Start work on reference delegates * Fix up tests to support adding documents at a sequence number * Implement removing references * Remove from target when dropped from local view * Fix warning * Add hooks for removal from mutation queue * Add hooks for limbo document updates * Style * Drop commented-out code * Fixup after merging master * Start importing delegate implementations * Memory LRU tests pass * Make memory lru work, start fixing up lru tests. All with hidden sequence numbers * Most LRU tests passing w/ memory * Big LRU test passes w/ memory * Leveldb LRU tests pass * All passing except local store tests * All tests pass * Fix localstore tests * Revert removeQueryData change * Remove collectGarbage calls * Remove NoOp GC * More cleanup * Refactor FSTHelpers * Cleanup and commenting * Comments and cleanup * Comments * Drop nullable for reference delegates, and old persistence method on spec tests * Fix missing sequence bump, remove unneeded targetID * Style * Drop commented-out code * Satisfy the linters and copyrighters, etc. * Drop more commented-out code * Fix import -> include, NSUInteger -> int * Start work on test comments * FSTDocumentKey -> DocumentKey * Sample of LRU test refactor (#1424) * Sample of test refactor * Drop errant 'struct' keyword * More test refactoring * Make gc a member variable too * Tests refactored to use member variables * Assign delegates procedurally, rather than passing in a block * Fix up FSTLevelDB a little * listens -> listenTargets * Rename GCEnabled * remove isSentinel * reword comment * Add case for non-eager GC to test * Style * Cleaning the lint trap * Clean up LRU tests (#1433) * Add copious helpers and some docs to lru tests * Style * Method name refactor, add comment * Drop FSTGarbageCollector and FSTEagerGarbageCollector (#1440) * Drop FSTGarbageCollector and FSTEagerGarbageCollector * Drop unused containsKey from FSTLevelDBMutationQueue * Style
* Include sequence number in FSTQueryData hash and isEqual methods * Style * Switch to util::Hash
* Add failing test * Move the restart tests to the leveldb test suite * Style * Add new persistence test helper * Fix test failure
wilhuff
approved these changes
Jul 27, 2018
| */ | ||
| class RollingSequenceNumberBuffer { | ||
| public: | ||
| explicit RollingSequenceNumberBuffer(NSUInteger max_elements) |
There was a problem hiding this comment.
This should be size_t max_elements as should the field below (since we're using it to compare against queue_.size())
| return queue_.top(); | ||
| } | ||
|
|
||
| std::size_t size() const { |
There was a problem hiding this comment.
nit: you can spell std::size_t as just size_t (we do so elsewhere).
| FSTListenSequenceNumber ReadSequenceNumber(const absl::string_view &slice) { | ||
| FSTListenSequenceNumber decoded; | ||
| absl::string_view tmp(slice.data(), slice.size()); | ||
| if (OrderedCode::ReadSignedNumIncreasing(&tmp, &decoded)) { |
There was a problem hiding this comment.
Super-ultra-mega-nitty-mcnitface: elsewhere we largely follow the pattern of
if (is-a-failure) {
return handle_failure();
}
proceed();
Here it doesn't matter now buf if we ever added anything to the affirmative case it would look weird. Consider reversing the clauses. (Feel free to ignore.)
| @end | ||
|
|
||
| @implementation FSTMemoryEagerReferenceDelegate { | ||
| std::unique_ptr<std::unordered_set<DocumentKey, DocumentKeyHash> > _orphaned; |
There was a problem hiding this comment.
With C++11, the lexical structure changed such that >> to close off nested templates is allowed. You no longer need to protect against this here.
| - (void)removeTarget:(FSTQueryData *)queryData { | ||
| for (const DocumentKey &docKey : | ||
| [_persistence.queryCache matchingKeysForTargetID:queryData.targetID]) { | ||
| self->_orphaned->insert(docKey); |
There was a problem hiding this comment.
Extraneous self->. (I only flag it because we're accessing _orphaned directly elsewhere in here.)
There was a problem hiding this comment.
Ah. It probably used to be in a block and then got migrated.
|
The build failure under Xcode 8 may be due to a missing |
added 2 commits
July 27, 2018 13:29
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Everything should have already been reviewed. This is the merge of the
LRUbookkeeping into master.