Basic Async Unary implementation of QPS benchmark #990

PeterAdams-A · 2020-10-13T09:32:52Z

Motivation:

The main grpc repo contains a benchmarking suite which can be implemented in any language.
This is an implementation of the async unary test and the framework surrounding it.

Notes:

Benchmark so far only run on OSX, not linux
Benchmark is not optimised - there are currently allocations within the benchmark which
could be removed.

Modifications:

Add framework for QPS style testing.
Add implementation of Async Unary benchmark (without TLS)
Add brief README describing how to run the benchmark

Result:

It's possible to run a benchmark of gRPC-swift which is in some way comparible to other
languages.

Motivation: The main grpc repo contains a benchmarking suite which can be implemented in any language. This is an implementation of the async unary test and the framework surrounding it. Notes: * Benchmark so far only run on OSX, not linux * Benchmark is not optimised - there are currently allocations within the benchmark which could be removed. Modifications: Add framework for QPS style testing. Add implementation of Async Unary benchmark (without TLS) Add brief README describing how to run the benchmark Result: It's possible to run a benchmark of gRPC-swift which is in some way comparible to other languages.

linux-foundation-easycla · 2020-10-13T09:32:54Z

The committers are authorized under a signed CLA.

✅ Peter Adams (c27ae51)

PeterAdams-A · 2020-10-13T13:32:36Z

Note: All the proto files in the Model directory are taken from grpc/grpc repo - all the swift to match is generated.

glbrntt

This looks mostly good. I've left a bunch of comments for tidying some stuff up. There are a couple of places where functions are quite long and have a lot of nesting which makes it pretty difficult to follow, I'd suggest splitting those up and also adding the odd blank link to break things up a bit. More documentation wouldn't go amiss either!

Also the formatting looks off in a bunch of places and is intended with 4 spaces and sometimes tabs. Running swiftformat . in the root of the repo should fix things up.

Performance/QPSBenchmark/Package.swift

Performance/QPSBenchmark/Sources/BenchmarkUtils/Histogram.swift

Performance/QPSBenchmark/Sources/QPSBenchmark/Runtime/AsyncClient.swift

Performance/QPSBenchmark/Sources/QPSBenchmark/Runtime/ClientUtilities.swift

glbrntt · 2020-10-13T14:04:22Z

Performance/QPSBenchmark/Makefile

+# Where products will be built; this is the SPM default.
+GRPC_SWIFT_PATH:=../..
+SWIFT_BUILD_PATH:=./.build
+SWIFT_BUILD_CONFIGURATION=debug


let's make this release

glbrntt · 2020-10-13T14:04:54Z

Performance/QPSBenchmark/Makefile

+SWIFT_BUILD_RELEASE:=${SWIFT} build ${SWIFT_FLAGS_RELEASE}
+SWIFT_TEST:=${SWIFT} test ${SWIFT_FLAGS}
+SWIFT_PACKAGE:=${SWIFT} package ${SWIFT_FLAGS}


Let's get rid of these too

This one hasn't been addressed.

Although I'm also wondering whether we should just piggyback on top of the main Makefile in the root of the repo to generate the protos. WDYT?

Its separate in the Package.swift - probably nice to keep the makefile separate too.

I've removed test and package.

glbrntt · 2020-10-13T14:05:22Z

Performance/QPSBenchmark/Makefile

+SWIFT_BUILD_CONFIGURATION=debug
+SWIFT_FLAGS=--build-path=${SWIFT_BUILD_PATH} --configuration=${SWIFT_BUILD_CONFIGURATION}
+# Force release configuration (for plugins)
+SWIFT_FLAGS_RELEASE=$(patsubst --configuration=%,--configuration=release,$(SWIFT_FLAGS))


Let's get rid of this

glbrntt · 2020-10-13T14:06:08Z

Performance/QPSBenchmark/Makefile

+SWIFT_PACKAGE:=${SWIFT} package ${SWIFT_FLAGS}
+
+# Name of generated xcodeproj
+XCODEPROJ:=GRPC.xcodeproj


And this one

glbrntt · 2020-10-13T14:07:33Z

Performance/QPSBenchmark/Makefile

+all:
+	${SWIFT_BUILD}
+
+.PHONY:
+plugins: ${PROTOC_GEN_SWIFT} ${PROTOC_GEN_GRPC_SWIFT}
+	cp $^ .


we can get rid of these too, I think

Co-authored-by: George Barnett <[email protected]>

PeterAdams-A · 2020-10-16T17:08:32Z

I've addressed most of your code review issues apart from the few things I've commented on above indicating I will not. I have added a tiny amount of extra docs as part of breaking up functions. Is there anything in particular you feel is definitely needing more doc?

glbrntt

Thanks for adding some more docs and splitting things up; the smaller functions help a lot.

This looks pretty good, I've left some comments mostly in AsyncClient which I think we can simplify a little.

glbrntt · 2020-10-19T08:30:38Z

Performance/QPSBenchmark/Sources/QPSBenchmark/Runtime/AsyncClient.swift

+    let allStopped = EventLoopFuture.andAllComplete(stoppedFutures, on: callbackLoop)
+    return allStopped.hop(to: callbackLoop).flatMap { _ in


Isn't allStopped already on callbackLoop -- that's what you're specifying in andAllComplete no?

Yes, looks like I over pulled fix from server side.

glbrntt · 2020-10-19T08:44:48Z

Performance/QPSBenchmark/Sources/QPSBenchmark/Runtime/AsyncClient.swift

+      while !self.stopRequested,
+        self.numberOfOutstandingRequests < self.maxPermittedOutstandingRequests {


minor, but can we replace this and the check in makeRequestAndRepeat? A computed property for "canMakeRequest" would avoid some duplication.

glbrntt · 2020-10-19T08:49:24Z

Performance/QPSBenchmark/Sources/QPSBenchmark/Runtime/AsyncClient.swift

+        self.stopIsComplete()
+      } else {
+        // Try scheduling another request.
+        try! self.launchRequests()


Can't we just call makeRequestAndRepeat here since we're only replacing one RPC?

In theory yes - it just felt nicer to make to perform a potential multiple request incase we've somehow lost one. I guess it costs a couple of compares so I'll change.

glbrntt · 2020-10-19T08:53:19Z

Performance/QPSBenchmark/Sources/QPSBenchmark/Runtime/AsyncClient.swift

+      return self.stats.copyData(reset: reset)
+    }
+
+    private static func makeClientRequest(payloadConfig: Grpc_Testing_PayloadConfig) throws


Does this need to be static? Also since the config doesn't change per ChannelRepeater why don't we just construct our request during init? That way we can drop the throws from launchRequests, makeRequestAndRepeat, and the try! from requestCompleted and start. We also avoid having to construct the request each time.

In fact, since AsyncQPSClient throws in its init we can make the request there and hand it to the ChannelRepeater

I think having looked at it, that this would make us more similar to the C version too.

glbrntt · 2020-10-19T08:56:30Z

Performance/QPSBenchmark/Sources/QPSBenchmark/Runtime/AsyncClient.swift

+    private var stopRequested = false
+    private var stopComplete: EventLoopPromise<Void>


I think these two would benefit from some documentation, stopping is a little unclear without reading all the code.

glbrntt · 2020-10-19T09:10:20Z

Performance/QPSBenchmark/Sources/QPSBenchmark/Runtime/BenchmarkServiceImpl.swift

+      // Making a payload which is not compressable is hard - and not implemented in
+      // other implementations too.


I guess they planned on doing this but never did and didn't want to break API 🤷‍♂️

glbrntt · 2020-10-19T09:20:28Z

Performance/QPSBenchmark/Sources/QPSBenchmark/Runtime/WorkerServiceImpl.swift

+  }
+
+  /// Create a client of the requested type.
+  private static func createClient(


nit: makeClient

glbrntt · 2020-10-19T09:22:04Z

Performance/QPSBenchmark/Sources/QPSBenchmark/Runtime/grpcTime.swift

+extension Nanoseconds: CustomStringConvertible {
+  /// Description to aid debugging.
+  var description: String {
+    return String(self.value)


Should we include the units too?

Suggested change

return String(self.value)

return "\(self.value) ns"

glbrntt · 2020-10-19T09:26:15Z

Performance/QPSBenchmark/Makefile

+SWIFT_BUILD_RELEASE:=${SWIFT} build ${SWIFT_FLAGS_RELEASE}
+SWIFT_TEST:=${SWIFT} test ${SWIFT_FLAGS}
+SWIFT_PACKAGE:=${SWIFT} package ${SWIFT_FLAGS}


This one hasn't been addressed.

Although I'm also wondering whether we should just piggyback on top of the main Makefile in the root of the repo to generate the protos. WDYT?

glbrntt · 2020-10-19T09:27:45Z

Performance/QPSBenchmark/Sources/QPSBenchmark/Runtime/main.swift

+    logger.info("Worker has finished.")
+  }
+}
+


Can we add in the following?

assert({ print("⚠️ WARNING: YOU ARE RUNNING IN DEBUG MODE ⚠️") return true }())

glbrntt

Looks great, just two nits remaining

glbrntt · 2020-10-19T13:54:20Z

Performance/QPSBenchmark/Sources/QPSBenchmark/Runtime/AsyncClient.swift

@@ -192,7 +238,7 @@ final class AsyncUnaryQPSClient: QPSClient {
        self.stopIsComplete()
      } else {
        // Try scheduling another request.
-        try! self.launchRequests()
+        try! self.makeRequestAndRepeat()


This function only throws because of this try on itself, let's drop both!

glbrntt · 2020-10-19T13:56:02Z

Performance/QPSBenchmark/Sources/QPSBenchmark/Runtime/AsyncClient.swift

        try self.makeRequestAndRepeat()
      }
    }

+    /// Returns if it is permissible to make another request - ie we've not been asked to stop, and we're not at the limit of outstanding requests.
+    private func canMakeRequest() -> Bool {


This should be a computed property:

private var canMakeRequest: Bool { return !(self.stopRequested || self.numberOfOutstandingRequests >= self.maxPermittedOutstandingRequests) }

glbrntt

LGTM thanks @PeterAdams-A!

PeterAdams-A added 2 commits October 13, 2020 13:19

Obey formatting conventions

6f71915

Ignore Package.resolved

72005b4

glbrntt requested changes Oct 13, 2020

View reviewed changes

PeterAdams-A and others added 13 commits October 14, 2020 14:41

Typo correction

e36ce4a

Co-authored-by: George Barnett <[email protected]>

Easy code review changes

11d9a64

Makefile code review comments

7fcd0f2

Fix test compilation

1a4408b

Move to using DispatchTime

a08dc32

Update commands re TLS support

9f5be1f

fix the eventloop changes for server shutdown.

635b2c3

Ensure we're on the correct event loop for shutdown

7789d4b

Break up long functions

e329141

Fix seconds converstion function

6802af8

Fix latency timings to have the correct units.

5df66c1

Formatting

dead6dc

Correct some comments.

4839a4c

PeterAdams-A requested a review from glbrntt October 16, 2020 17:07

glbrntt requested changes Oct 19, 2020

View reviewed changes

PeterAdams-A added 4 commits October 19, 2020 12:31

Remove unused lines

f90d128

Code review changes.

9a821f9

formatting changes

6d7d21b

Last couple of code review changes.

0d4a5bb

PeterAdams-A requested a review from glbrntt October 19, 2020 13:43

glbrntt requested changes Oct 19, 2020

View reviewed changes

More review changes.

fb987c5

PeterAdams-A requested a review from glbrntt October 19, 2020 15:10

glbrntt approved these changes Oct 19, 2020

View reviewed changes

glbrntt added the semver/none No version bump required. label Oct 20, 2020

glbrntt merged commit 4f4d98a into grpc:main Oct 20, 2020

glbrntt mentioned this pull request Nov 24, 2020

Support standard benchmarking #976

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic Async Unary implementation of QPS benchmark #990

Basic Async Unary implementation of QPS benchmark #990

PeterAdams-A commented Oct 13, 2020

linux-foundation-easycla bot commented Oct 13, 2020 •

edited

Loading

PeterAdams-A commented Oct 13, 2020

glbrntt left a comment

glbrntt Oct 13, 2020

glbrntt Oct 13, 2020

glbrntt Oct 19, 2020

PeterAdams-A Oct 19, 2020

glbrntt Oct 13, 2020

glbrntt Oct 13, 2020

glbrntt Oct 13, 2020

PeterAdams-A commented Oct 16, 2020

glbrntt left a comment

glbrntt Oct 19, 2020

PeterAdams-A Oct 19, 2020

glbrntt Oct 19, 2020

glbrntt Oct 19, 2020

PeterAdams-A Oct 19, 2020

glbrntt Oct 19, 2020

glbrntt Oct 19, 2020

PeterAdams-A Oct 19, 2020

glbrntt Oct 19, 2020

glbrntt Oct 19, 2020

glbrntt Oct 19, 2020

glbrntt Oct 19, 2020

glbrntt Oct 19, 2020

glbrntt Oct 19, 2020

glbrntt left a comment

glbrntt Oct 19, 2020

glbrntt Oct 19, 2020

glbrntt left a comment

		let allStopped = EventLoopFuture.andAllComplete(stoppedFutures, on: callbackLoop)
		return allStopped.hop(to: callbackLoop).flatMap { _ in

		while !self.stopRequested,
		self.numberOfOutstandingRequests < self.maxPermittedOutstandingRequests {

		private var stopRequested = false
		private var stopComplete: EventLoopPromise<Void>

		// Making a payload which is not compressable is hard - and not implemented in
		// other implementations too.

Basic Async Unary implementation of QPS benchmark #990

Basic Async Unary implementation of QPS benchmark #990

Conversation

PeterAdams-A commented Oct 13, 2020

linux-foundation-easycla bot commented Oct 13, 2020 • edited Loading

PeterAdams-A commented Oct 13, 2020

glbrntt 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

PeterAdams-A commented Oct 16, 2020

glbrntt 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

glbrntt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glbrntt left a comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Oct 13, 2020 •

edited

Loading