Optimize Artery RemoteInstrument #21924

Merged
merged 2 commits into from Dec 6, 2016

Projects

None yet

4 participants

@bantonsson
Member

This is a cleanup and optimization of the RemoteInstrument code in Artery.

  • Change RemoteInstrument API to work with ByteBuffers
  • Remove the MetadataMap since serialization is done in buffer

Here are some unscientific benchmark numbers from my laptop.

----------------------------------------------------------------------------------------------------------------------
| Benchmark                                         |     (configType) |  Mode | Cnt |    Score |     Error |  Units |
|---------------------------------------------------|------------------|-------|-----|----------|-----------|--------|
| Old                                               |                  |       |     |          |           |        |
| CodecBenchmark.both                               |         Standard | thrpt |   5 |  750.592 | ± 103.790 | ops/ms |
| CodecBenchmark.both:·gc.churn.PS_Eden_Space       |         Standard | thrpt |   5 |  475.762 | ± 608.715 | MB/sec |
| CodecBenchmark.both:·gc.churn.PS_Eden_Space.norm  |         Standard | thrpt |   5 |  772.095 | ±  16.517 |   B/op |
| CodecBenchmark.both                               | RemoteInstrument | thrpt |   5 |  573.369 | ± 190.089 | ops/ms |
| CodecBenchmark.both:·gc.churn.PS_Eden_Space       | RemoteInstrument | thrpt |   5 |  662.329 | ± 714.277 | MB/sec |
| CodecBenchmark.both:·gc.churn.PS_Eden_Space.norm  | RemoteInstrument | thrpt |   5 | 1420.378 | ±  12.754 |   B/op |
| New                                               |                  |       |     |          |           |        |
| CodecBenchmark.both                               |         Standard | thrpt |   5 |  854.291 | ± 166.186 | ops/ms |
| CodecBenchmark.both:·gc.churn.PS_Eden_Space       |         Standard | thrpt |   5 |  533.664 | ± 691.398 | MB/sec |
| CodecBenchmark.both:·gc.churn.PS_Eden_Space.norm  |         Standard | thrpt |   5 |  758.389 | ±  20.873 |   B/op |
| CodecBenchmark.both                               | RemoteInstrument | thrpt |   5 |  791.343 | ± 170.640 | ops/ms |
| CodecBenchmark.both:·gc.churn.PS_Eden_Space       | RemoteInstrument | thrpt |   5 |  516.096 | ± 652.720 | MB/sec |
| CodecBenchmark.both:·gc.churn.PS_Eden_Space.norm  | RemoteInstrument | thrpt |   5 |  778.509 | ±   8.818 |   B/op |
----------------------------------------------------------------------------------------------------------------------

The new RemoteInstrument is only slightly slower and consumes roughly the same number of bytes per op as the standard Artery code.

BTW, the materialization of the Streams still happen inside the measured part, even if I move everything else out. That happens several times during a single run. Would be nice to get that out of the way as well, but it's for another time.

@bantonsson bantonsson =jmh Clean up Artery CodecBenchmark
* Make decoding release the envelope so it doesn't leak memory
* Create two different configurations, Standard and RemoteInstrument
* Create Serilaizer and RemoteInstrument that doesn't allocate memory
f93d14a
@akka-ci
Member
akka-ci commented Dec 1, 2016

Test PASSed.

@akka-ci akka-ci removed the validating label Dec 1, 2016
@ktoso
Member
ktoso commented Dec 2, 2016

LGTM, I had reviewed this before.

@ktoso
ktoso approved these changes Dec 2, 2016 View changes
@bantonsson
Member

Any more opinions on this one?

@patriknw
Member
patriknw commented Dec 5, 2016

I'll review it.

@patriknw

LGTM, minor nitpick

+ * Called right before putting the message onto the wire.
+ * Parameters MAY be `null` (except `message` and `buffer`)!
+ */
+ def remoteMessageSent(recipient: ActorRef, message: Object, sender: ActorRef, size: Int, time: Long): Unit
@patriknw
patriknw Dec 5, 2016 Member

add some doc about what size and time are and cross reference to serializationTimingEnabled

@bantonsson
bantonsson Dec 6, 2016 Member

Absolutely. I'll add some docs.

+ } else {
+ log.debug(
+ "Skipping serialized data in message for RemoteInstrument(s) {} that has no local match",
+ remoteInstrumentIdIteratorRaw(buffer, endPos).mkString("[", ", ", "]"))
@patriknw
patriknw Dec 5, 2016 Member

is this costly? perhaps add if (log.isDebugEnabled)?

@bantonsson
bantonsson Dec 6, 2016 Member

You're right. The mkString is evaluated directly here. I'll fix it. I wonder why I was under the impression that it was call by name?

@bantonsson bantonsson =rem Streamline Artery encoding of metadata to avoid allocations
* Change RemoteInstrument API to work with ByteBuffers
* Remove the MetadataMap since serialization is done in buffer
5d8a153
@akka-ci akka-ci added validating and removed tested labels Dec 6, 2016
@akka-ci akka-ci added tested and removed validating labels Dec 6, 2016
@akka-ci
Member
akka-ci commented Dec 6, 2016

Test PASSed.

@bantonsson bantonsson merged commit 8d05592 into akka:master Dec 6, 2016

2 checks passed

default Test PASSed.
Details
typesafe-cla-validator All users have signed the CLA
Details
@bantonsson bantonsson deleted the bantonsson:optimize-artery-remote-instrument branch Dec 6, 2016
@patriknw
Member
patriknw commented Dec 6, 2016

Thanks, @bantonsson !

@patriknw
Member
patriknw commented Dec 6, 2016

Refs #21482

@patriknw patriknw added this to the 2.5.0 milestone Dec 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment