Eliminate unnecessary DocumentKey->FSTDocumentKey conversions #1507
Conversation
| const long long memoryUsedAfterCommitMb = getCurrentMemoryUsedInMb(); | ||
| XCTAssertNotEqual(memoryUsedAfterCommitMb, -1); | ||
| const long long memoryDeltaMb = memoryUsedAfterCommitMb - memoryUsedBeforeCommitMb; | ||
| // This by its nature cannot be a precise value. Runs on simulator seem to give an increase of |
There was a problem hiding this comment.
This function only differs from #1505 in lines 368:370 (the comment and the constant).
| const auto errorCode = | ||
| task_info(mach_task_self(), MACH_TASK_BASIC_INFO, (task_info_t)&taskInfo, &taskInfoSize); | ||
| if (errorCode == KERN_SUCCESS) { | ||
| const int bytesInMegabyte = 1000 * 1000; |
There was a problem hiding this comment.
nit: 1024 * 1024? not that much difference, up to you.
There was a problem hiding this comment.
Done. I guess I used the SI definition.
| FIRWriteBatch *batch = [mainDoc.firestore batch]; | ||
|
|
||
| // >= 500 mutations will be rejected, so use 500-1 mutations | ||
| for (int i = 0; i != 500 - 1; ++i) { |
There was a problem hiding this comment.
Could we define a constant i.e. give the literal 500 a name?
|
|
||
| NS_ASSUME_NONNULL_BEGIN | ||
|
|
||
| @interface FSTDocumentKey () { | ||
| /** The path to the document. */ | ||
| ResourcePath _path; | ||
| DocumentKey _impl; |
There was a problem hiding this comment.
This may be better named _delegate. I'm not a fan of "impl" as a name since it's generic without suggesting how it's the implementation.
There was a problem hiding this comment.
Is there still any usage of implicit conversion? If not, could we remove the two functions that does the implicit conversion (each direction) from the .cc file; or otherwise update the implementation there according to the change made in this PR?
Unfortunately, there's still too many usages to tackle in this PR.
|
|
||
| NS_ASSUME_NONNULL_BEGIN | ||
|
|
||
| @interface FSTDocumentKey () { | ||
| /** The path to the document. */ | ||
| ResourcePath _path; | ||
| DocumentKey _impl; |
| const auto errorCode = | ||
| task_info(mach_task_self(), MACH_TASK_BASIC_INFO, (task_info_t)&taskInfo, &taskInfoSize); | ||
| if (errorCode == KERN_SUCCESS) { | ||
| const int bytesInMegabyte = 1000 * 1000; |
There was a problem hiding this comment.
Done. I guess I used the SI definition.
| FIRWriteBatch *batch = [mainDoc.firestore batch]; | ||
|
|
||
| // >= 500 mutations will be rejected, so use 500-1 mutations | ||
| for (int i = 0; i != 500 - 1; ++i) { |
| @@ -348,7 +348,8 @@ - (void)testReasonableMemoryUsageForLotsOfMutations { | |||
| FIRWriteBatch *batch = [mainDoc.firestore batch]; | |||
|
|
|||
| // >= 500 mutations will be rejected, so use 500-1 mutations | |||
| for (int i = 0; i != 500 - 1; ++i) { | |||
| const int maxMutations = 500 - 1; | |||
There was a problem hiding this comment.
I was actually off by one on this, it's >500 mutations that are rejected, not >=500.
This came up while optimizing batches with many mutations. Most of memory allocations and a good chunk of time was due to many unintentional conversions between the C++
DocumentKeyand Objective-CFSTDocumentKey. There were two main issues:DocumentKeytoFSTDocumentKeyperformed a full copy, instead of taking advantage of the internal optimization inDocumentKeythat uses ashared_ptrto make copying cheap;FSTDocumentKeywere replaced withDocumentKey, but the syntax for comparison wasn't updated. Because the conversion is implicit, the following code convertsDocumentKeys toFSTDocumentKeys and then calls the method on them:DocumentKey foo; DocumentKey bar; return [foo isEqualToKey: bar];This is a follow-up to #1505. I found out that while #1505 improved things a lot, committing more than one batch with many mutations was still unreasonably slow. The profiler showed that most of the time was spent in creating
FSTDocumentKeys. To benchmark locally, I used the newly-added performance test which commits a batch with 499 mutations, but created 50 such batches. I tested in release mode and just manually monitored memory usage and execution speed in XCode (on simulator). I got the following stats:Further optimizations to
FSTLocalDocumentsViewseem not to be worth it. Even skipping access to LevelDB altogether insideFSTLocalDocumentsViewonly brings down total time to 19-20 seconds.