Benchmark for ZMX supervisor

[KadekM]

#104

[KadekM]

some numbers:

TrackingFibersBenchmark.defaultBroad   100000  thrpt   25  825816.549 ± 14328.893  ops/s
TrackingFibersBenchmark.zmxBroad       100000  thrpt   25  630539.903 ± 24254.122  ops/s

TrackingFibersBenchmark.defaultDeep    100000  thrpt   25   81304.054 ±   528.297  ops/s
TrackingFibersBenchmark.zmxDeep        100000  thrpt   25   69800.148 ±   393.147  ops/s

TrackingFibersBenchmark.defaultMixed  100000  thrpt   25  689234.657 ± 20727.947  ops/s
TrackingFibersBenchmark.zmxMixed      100000  thrpt   25  572079.955 ± 22287.688  ops/s

[adamgfraser]

Looks great! A couple thoughts:

1. In the broad and mixed benchmarks we may not actually be creating a fiber graph that is that large at one point in time because even though we are forking all these fibers at once each fiber is going to terminate almost immediately, so some may terminate before others are forked. Could be good to do something like fork a bunch of fibers that never complete to really make sure we are generated a graph that contains 100,000 elements
2. Could be good to add a couple of benchmarks where we generate a set of all the fibers to test performance of reading from the graph in addition to writing to it.

[KadekM]

Thanks, I'll take a look in the next few days

[jdegoes]

@KadekM Any interest in reviving this PR? If so, please re-open. I think benchmarking ZMX to ensure we don't suffer significant performance degradation on large graphs is quite important!

[KadekM]

@jdegoes sure, it had slipped my mind

[adamgfraser]

@KadekM Great!

[KadekM]

@adamgfraser
as for 1), I've changed the fibers to not terminate
as for 2), I've added some tests on reading tiny graphs (n < 100), not sure if that's what you had in mind, but it crashes for larger graphs on StackOverflow @ [error] at zio.zmx.diagnostics.graph.Graph.fold(Graph.scala:172)

[adamgfraser]

@KadekM Interesting.

I think we are actually going to delete the graph representation itself and just keep track of the set of fibers. See #209. Maybe you can pull in the changes to the ZMXSupervisor from there since it is not going to be very meaningful to benchmark very small programs like that.

The other thing you could do if you wanted would be to add benchmarks for the overhead of recording user specified metrics, though we could also definitely do that separately.

[CLAassistant]

All committers have signed the CLA.

[KadekM]

@adamgfraser pulled in those changes, it looks OK now (no more SO).

The other thing you could do if you wanted would be to add benchmarks for the overhead of recording user specified metrics, though we could also definitely do that separately.

I'd prefer if we do it separately, this one's been open for a while

[adamgfraser]

A couple of minor comments on the benchmarks. Otherwise this is ready to go when tests pass. What do the numbers look like?

[KadekM]

I'll run them once you're ok with benchmark setup, as they take a while to finish

[adamgfraser]

👍 One thing I find is useful for local development is running benchmarks with a smaller number of iterations and forks (like 1 fork, five warmup and benchmark iterations, 3 seconds each) just to make sure things are working right and the results look broadly reasonable. Though definitely after that good to run with a large number of forks and iterations for more accurate results.

[KadekM]

I did run them one iteration each to make sure the number seem reasonable and nothing fishy is happening, like that SO :-) before posting them here though I'd like to run it properly

[adamgfraser]

Makes sense!

[KadekM]

Updated based on comments in cab61eb

starting tests

[KadekM]

@adamgfraser how do we want to handle

zio-zmx/src/main/scala/zio/zmx/diagnostics/package.scala:18:24: 
object JavaConverters in package collection is deprecated (since 2.13.0): Use `scala.jdk.CollectionConverters` instead
[error]         UIO.succeedNow(fibers.asScala.toSet)`

[adamgfraser]

@KadekM I think you can use silencer to silence it. Example usage:

import com.github.ghik.silencer.silent

val locals = fiberRefLocals.asScala: @silent("JavaConverters")

[KadekM]

The spawn (for large enough n) also crashes on SO so I'll need to fix that

[KadekM]

@adamgfraser apologies it took so long, here are the first results after changes got some 'interesting results'
jmh:run -i 1 -wi 1

[info] ReadingGraphBroadBenchmarks.readingBroad  100000  thrpt    5   76932.833 ± 79619.602  ops/s
[info] ReadingGraphDeepBenchmarks.readingDeep    100000  thrpt    5  121838.864 ± 23600.871  ops/s
[info] ReadingGraphMixedBenchmarks.readingMixed  100000  thrpt    5   34172.760 ± 50229.856  ops/s
[info] TrackingFibersBenchmark.defaultBroad      100000  thrpt    5       0.634 ±     0.041  ops/s
[info] TrackingFibersBenchmark.defaultDeep       100000  thrpt    5       0.575 ±     0.080  ops/s
[info] TrackingFibersBenchmark.defaultMixed      100000  thrpt    5       0.070 ±     0.007  ops/s
[info] TrackingFibersBenchmark.zmxBroad          100000  thrpt    5       0.614 ±     0.041  ops/s
[info] TrackingFibersBenchmark.zmxDeep           100000  thrpt    5       0.603 ±     0.048  ops/s
[info] TrackingFibersBenchmark.zmxMixed          100000  thrpt    5       0.067 ±     0.011  ops/s

it seems in reading the deviation is quite big
for the tracking maybe we are creating graphs that are too large ? I tried it with 10k

[info] Benchmark                            (size)   Mode  Cnt  Score   Error  Units
[info] TrackingFibersBenchmark.defaultDeep   10000  thrpt    5  8.082 ± 1.673  ops/s
[info] TrackingFibersBenchmark.zmxDeep       10000  thrpt    5  7.521 ± 0.827  ops/s

here are tracking for 10k with jmh:run -i 3 -wi 3:

[info] Benchmark                             (size)   Mode  Cnt   Score   Error  Units
[info] TrackingFibersBenchmark.defaultBroad   10000  thrpt   15   8.068 ± 0.145  ops/s
[info] TrackingFibersBenchmark.zmxBroad       10000  thrpt   15   7.846 ± 0.187  ops/s

[info] TrackingFibersBenchmark.defaultDeep    10000  thrpt   15   7.840 ± 0.283  ops/s
[info] TrackingFibersBenchmark.zmxDeep        10000  thrpt   15   7.226 ± 0.853  ops/s

[info] TrackingFibersBenchmark.defaultMixed   10000  thrpt   15  19.992 ± 0.518  ops/s
[info] TrackingFibersBenchmark.zmxMixed       10000  thrpt   15  17.814 ± 0.396  ops/s

[adamgfraser]

I don't think the ReadingGraph benchmark is really telling us anything at this point. All we are doing when we call ZMXSupervisor.value is reading a set so I think that is being dwarfed by the noise of how far the Future has gotten in execution. I think I would just delete that since we are planning on removing the graph anyway. The tracking benchmark seems pretty encouraging in that it indicates there is only a 5% to 10% overhead for tracking fibers in this microbenchmark. In real applications when you are doing more than just forking fibers that don't do anything that cost is going to be even lower. So I think that for at least that part of this functionality we can feel good about our efficiency there.

[KadekM]

@adamgfraser I've deleted the reading ones

[KadekM]

@adamgfraser subtle ping :-)

[adamgfraser]

@KadekM Sorry for the delay! Yes, I think we shouldn't change the API beyond just removing the graph which is not actually being used.

[KadekM]

@adamgfraser Diagnostics has been resurrected :-)

[adamgfraser]

I think now we have two Diagnostics. Good to go once compilation issues are resolved.

[adamgfraser]

@KadekM Thank you for your work on this!

[KadekM]

Pushing code before morning coffee - never again :D

[KadekM]

@adamgfraser Thanks for constant back and forths with reviews