Implement byteSize methods on lru delegates for use in GC threshold #1893
Conversation
| @@ -16,6 +16,7 @@ | |||
|
|
|||
| #import <Foundation/Foundation.h> | |||
|
|
|||
| #import "Firestore/Source/Local/FSTLocalSerializer.h" | |||
There was a problem hiding this comment.
Conventionally in Objective-C we've been using forward declarations (with @class) to keep compilation times down.
Note that this works pretty well in Objective-C but basically doesn't work for C++ so we use the IWYU rule there.
There was a problem hiding this comment.
Ok, switched to a forward declaration, here and elsewhere.
| auto iter = util::DirectoryIterator::Create(_directory); | ||
| for (; iter->Valid(); iter->Next()) { | ||
| off_t fileSize = util::FileSize(iter->file()).ValueOrDie(); | ||
| count += fileSize; |
There was a problem hiding this comment.
Strictly speaking this isn't safe: size_t is a type that measures the sizes of objects in memory, so on 32-bit systems it will be 32 bits. off_t is the size of files on disk, and can be defined to be 64 bits even on 32 bit systems (e.g. by passing -D_FILE_OFFSET_BITS=64 to the build).
However, I'm not sure I care about supporting local caches larger than 4GB on 32 bit systems and I can't see us exposing the end user control of the cache size in terms of off_t so it seems silly to internally represent the cache size that way only to knee cap it through the control knob.
Maybe just assert we don't overflow here?
There was a problem hiding this comment.
I switched the accumulator to off_t and assert that it is less than SIZE_MAX before returning.
Firestore/Source/Local/FSTLevelDB.mm
Outdated
| @@ -16,6 +16,8 @@ | |||
|
|
|||
| #import "Firestore/Source/Local/FSTLevelDB.h" | |||
|
|
|||
| #include <dirent.h> | |||
There was a problem hiding this comment.
These headers aren't portable.
Better to allow filesystem.h to provide off_t however it's going to get it.
There was a problem hiding this comment.
Removed. I think they were from a previous iteration of this anyways, and are no longer needed.
| @@ -103,4 +105,6 @@ extern const firebase::firestore::model::ListenSequenceNumber kFSTListenSequence | |||
| - (int)removeOrphanedDocumentsThroughSequenceNumber: | |||
| (firebase::firestore::model::ListenSequenceNumber)sequenceNumber; | |||
|
|
|||
| - (size_t)onDiskSize; | |||
There was a problem hiding this comment.
Could this name be the same as byteSize above?
| @@ -466,6 +469,14 @@ - (NSUInteger)indexOfExistingBatchID:(BatchId)batchID action:(NSString *)action | |||
| return (NSUInteger)index; | |||
| } | |||
|
|
|||
| - (size_t)byteSizeWithSerializer:(FSTLocalSerializer *)serializer { | |||
| __block size_t count = 0; | |||
| [self.queue enumerateObjectsUsingBlock:^(FSTMutationBatch *batch, NSUInteger idx, BOOL *stop) { | |||
There was a problem hiding this comment.
You can use a for-in loop:
size_t count = 0;
for (FSTMutationBatch* batch in self.queue) {
count += [[[serializer encodedMutationBatch:batch] data] length];
}| static size_t FSTDocumentKeyByteSize(FSTDocumentKey *key) { | ||
| size_t count = 0; | ||
| for (auto it = key.path.begin(); it != key.path.end(); it++) { | ||
| count += (*it).size(); |
There was a problem hiding this comment.
How exact is this meant to be? At the very least it's excluding the path separators.
There was a problem hiding this comment.
I've updated the comment to reflect that this is an estimate. It's for memory persistence, so it doesn't have to be exact, and furthermore I don't think we actually store the separators in memory? I believe it's a vector of segments, or something like that.
These methods will be used to determine if we should run LRU collection.