Add Google Benchmark library and benchmark leveldb #1137

gsoltis · 2018-04-18T21:00:34Z

Adds benchmark capability. Includes a benchmark of leveldb writes with and without index writes. The test is currently disabled to avoid running by default.

wilhuff

Very cool!

See below for an alternative arrangement that doesn't require checking in dead code.

wilhuff · 2018-04-18T21:12:04Z

Firestore/Example/Tests/Local/FSTLevelDBBenchmarkTests.mm

+
+- (void)testRunBenchmarks {
+  // Enable to run benchmarks.
+  if (false) {


This is probably better captured as an early return so that this thing ends up written at a normal indent.

Also a better way to make this work is to create a new Benchmarks_iOS target within the project and add this test to that target instead of Firestore_Tests. That way we don't have to edit the files to run the benchmarks and they naturally exclude themselves during normal work.

gsoltis · 2018-07-19T16:44:02Z

Ok, I made a separate target and it runs the benchmarks. No more dead code. PTAL.

wilhuff

Basically LGTM

wilhuff · 2018-07-19T18:04:54Z

Firestore/Example/Firestore.xcodeproj/project.pbxproj

@@ -709,6 +743,7 @@
 				6003F5B5195388D20070C39A /* Tests */,
 				54764FAC1FAA0C390085E60A /* CoreTests */,
 				54C9EDF22040E16300A969CD /* SwiftTests */,
+				5CAE131A20FFFED600BE9A4A /* Firestore_Benchmarks_iOS */,


Could rename this in the left-hand nav to just "Benchmarks"?

wilhuff · 2018-07-19T18:06:36Z

Firestore/Example/Firestore_Benchmarks_iOS/FSTLevelDBBenchmarkTests.mm

+ */
+
+#import <Foundation/Foundation.h>
+#import "../../../../../../../Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/Library/Frameworks/XCTest.framework/Headers/XCTest.h"


This should be <XCTest/XCTest.h>.

wilhuff · 2018-07-19T18:07:41Z

Firestore/Example/Podfile

+  target 'Firestore_Benchmarks_iOS' do
+    inherit! :search_paths
+
+    pod 'GoogleBenchmark', :podspec => 'Tests/GoogleBenchmark/GoogleBenchmark.podspec'


Please put this podspec just directly within Firestore/Example (like ProtobufCpp and GoogleTest above).

wilhuff · 2018-07-19T18:08:50Z

Firestore/Example/Tests/GoogleBenchmark/GoogleBenchmark.podspec

+      'include/benchmark/*.h'
+  ]
+
+  s.pod_target_xcconfig = {


I think if you add "${PODS_ROOT}/GoogleBenchmark/include" to pod_user_config you won't have to manually configure that.

Leaving this as-is. The other setting did not work, and this currently matches GoogleTest.

var-const

Some optional nitpicks (feel free to ignore), otherwise LGTM.

var-const · 2018-07-19T18:09:29Z

Firestore/Example/Firestore_Benchmarks_iOS/FSTLevelDBBenchmarkTests.mm

+ */
+
+#import <Foundation/Foundation.h>
+#import "../../../../../../../Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/Library/Frameworks/XCTest.framework/Headers/XCTest.h"


Can this be just #import <XCTest/XCTest.h>; or similar?

var-const · 2018-07-19T18:10:35Z

Firestore/Example/Podfile

@@ -18,6 +18,11 @@ target 'Firestore_Example_iOS' do
  pod 'GoogleUtilities', :path => '../../'
  pod 'FirebaseFirestore', :path => '../../'

+#  pod 'leveldb-library'


Can this commented-out code be removed?

var-const · 2018-07-19T18:29:54Z

Firestore/Example/Tests/GoogleBenchmark/GoogleBenchmark.podspec

+  }
+
+  s.library = 'c++'
+end


Ultranit: add a newline at the end of the file, so that Github doesn't display the annoying red symbol?

var-const · 2018-07-19T18:34:57Z

Firestore/Example/Firestore_Benchmarks_iOS/FSTLevelDBBenchmarkTests.mm

+using firebase::firestore::model::DatabaseId;
+
+// Pre-existing document size
+static const int kDocumentSize = 1024 * 2;  // 2 kb


Optional nit: instead of using static variables and functions, consider wrapping them in an anonymous namespace. It achieves the same thing, but is slightly less verbose, and has the advantage of working being more universal (static cannot be applied to class/struct).

var-const · 2018-07-19T18:36:06Z

Firestore/Example/Firestore_Benchmarks_iOS/FSTLevelDBBenchmarkTests.mm

+}
+
+static NSString *LevelDBDir() {
+  NSError *error;


Optional nit: move error closer to the point of usage, in the if statement scope?

var-const · 2018-07-19T18:43:51Z

Firestore/Example/Firestore_Benchmarks_iOS/FSTLevelDBBenchmarkTests.mm

+            _emptyBuffer);
+  }
+
+  FSTLevelDB *_db;


I'm not sure what the rules are for a pointer to an Objective-C object. A regular pointer would be uninitialized, so I'd suggest setting it to nullptr. However, perhaps this pointer is set to nil automatically?

It is automatically nil: https://stackoverflow.com/a/9072040

var-const · 2018-07-19T18:46:00Z

Firestore/Example/Firestore_Benchmarks_iOS/FSTLevelDBBenchmarkTests.mm

+// In each test, either overwrite index entries and documents, or just documents
+
+BENCHMARK_DEFINE_F(LevelDBFixture, RemoteEvent)(benchmark::State &state) {
+  bool writeIndexes = (bool)state.range(0);


Optional nit: use static_cast<bool> instead of C-style conversion, it's easier to spot, greppable, and it's more clear what it does (C-style cast is much more powerful than a static_cast, which may lead to surprising unintended conversions, especially when code is modified later on).

var-const · 2018-07-19T18:46:33Z

Firestore/Example/Firestore_Benchmarks_iOS/FSTLevelDBBenchmarkTests.mm

+BENCHMARK_DEFINE_F(LevelDBFixture, RemoteEvent)(benchmark::State &state) {
+  bool writeIndexes = (bool)state.range(0);
+  int64_t documentSize = state.range(1);
+  int64_t docsToUpdate = state.range(2);


Nit: add #include <cstdint>;?

var-const · 2018-07-19T18:46:50Z

Firestore/Example/Firestore_Benchmarks_iOS/FSTLevelDBBenchmarkTests.mm

+  bool writeIndexes = (bool)state.range(0);
+  int64_t documentSize = state.range(1);
+  int64_t docsToUpdate = state.range(2);
+  std::string documentUpdate = UpdatedDocumentData((int)documentSize);


Same optional nit about casting. Or perhaps just modify UpdatedDocumentData to accept an int64_t? There doesn't seem to be a need for divergence.

var-const · 2018-07-19T18:48:53Z

Firestore/Example/Firestore_Benchmarks_iOS/FSTLevelDBBenchmarkTests.mm

+  int64_t documentSize = state.range(1);
+  int64_t docsToUpdate = state.range(2);
+  std::string documentUpdate = UpdatedDocumentData((int)documentSize);
+  for (auto _ : state) {


Can you make the loop variable a const reference? for (const auto& _ : state) {. While _ variable is not used, copies are still being made. I don't know what state contains, but presumably these are objects larger than a couple of ints (otherwise, just leave as is).

var-const

Thanks for doing the changes!

wilhuff

LGTM

Greg Soltis added 7 commits April 17, 2018 16:52

Step one of benchmarking

9773ceb

Benchmark writing to leveldb

d7d233e

Comment out and style

516c066

More comments

15ed746

Fix warnings

a8c225a

Someday I will lint as good as computer

520433a

Some light commenting and static-ing

b8fc089

gsoltis assigned wilhuff Apr 18, 2018

gsoltis requested a review from wilhuff April 18, 2018 21:00

googlebot added the cla: yes label Apr 18, 2018

wilhuff suggested changes Apr 18, 2018

View reviewed changes

wilhuff assigned gsoltis and unassigned wilhuff Apr 20, 2018

wilhuff added the api: firestore label Apr 20, 2018

Merge in masteR

b6e91aa

var-const mentioned this pull request Jul 13, 2018

Minimize rereading of batches in FSTLocalDocumentsView documentsMatchingCollectionQuery #1533

Merged

Greg Soltis added 3 commits July 18, 2018 15:53

Merge master

00f2757

It seems to be working?

a172a52

Remove unused test file

57b0ba3

gsoltis assigned var-const and wilhuff and unassigned gsoltis Jul 19, 2018

wilhuff reviewed Jul 19, 2018

View reviewed changes

wilhuff assigned gsoltis and unassigned var-const and wilhuff Jul 19, 2018

var-const approved these changes Jul 19, 2018

View reviewed changes

Greg Soltis added 2 commits July 19, 2018 14:04

Drop commented-out deps

a6448ae

Revert string quoting in pbxproj file

c7d305c

Greg Soltis added 5 commits July 19, 2018 14:37

Drop reference to deleted file

97df84d

Move podspec, fix import

64a3f64

Lots of C++ style changes

8d068f3

Merge master

914daeb

Style

cb918df

var-const approved these changes Jul 19, 2018

View reviewed changes

wilhuff approved these changes Jul 20, 2018

View reviewed changes

Drop whitespace

34ce92d

gsoltis merged commit 3b939a2 into master Jul 20, 2018

gsoltis deleted the gsoltis/benchmark_leveldb branch July 20, 2018 02:44

firebase locked and limited conversation to collaborators Nov 4, 2019

Add Google Benchmark library and benchmark leveldb #1137

Add Google Benchmark library and benchmark leveldb #1137

Conversation

gsoltis commented Apr 18, 2018

wilhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsoltis commented Jul 19, 2018

wilhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

var-const left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

var-const left a comment

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment