Optimize metrics wrapper

[kyri-petrou]

Fairly certain we can optimise it further, but at least now it's not cripplingly slow. Open to suggestions on what else can be improved. The metrics wrapper is now ~x50 faster but still twice as slow as the ApolloTracing wrapper.

Before:

[info] Benchmark                                      Mode  Cnt    Score    Error  Units
[info] NestedZQueryBenchmark.apolloTracingBenchmark  thrpt    5  202.018 ± 21.100  ops/s
[info] NestedZQueryBenchmark.metricsBenchmark        thrpt    5    5.500 ±  0.258  ops/s
[info] NestedZQueryBenchmark.noWrappersBenchmark     thrpt    5  511.841 ±  8.066  ops/s

After

[info] Benchmark                                      Mode  Cnt    Score   Error  Units
[info] NestedZQueryBenchmark.apolloTracingBenchmark  thrpt    5  199.308 ± 3.427  ops/s
[info] NestedZQueryBenchmark.metricsBenchmark        thrpt    5  117.211 ± 3.722  ops/s
[info] NestedZQueryBenchmark.noWrappersBenchmark     thrpt    5  515.891 ± 7.015  ops/s

If we fork the writing of metrics, then the metricsBenchmark becomes faster than the ApolloTracing wrapper:

[info] Benchmark                                Mode  Cnt    Score   Error  Units
[info] NestedZQueryBenchmark.metricsBenchmark  thrpt    5  213.406 ± 4.114  ops/s

[kyri-petrou]

I'm not sure how I feel about forking this effect, but it's by far the most performant implementation. With forking, it becomes faster than the ApolloTracing wrapper (see performance result in PR description)

[ghostdogpr]

Makes sense to not make the execution slower because we're updating the metrics. And since this wrapper does't wrap pure fields, we don't create too many useless fibers.

[kyri-petrou]

By the way we're forking only once per query execution here, so the effect of the forking itself should be extremely minimal. My only concern is that in the very unlikely case that the calculation of offsets + writing of metrics is slower than the query execution, this could lead to a memory leak.

However, the chances of that happening are extremely low since in almost all cases the query execution will be slower than this

[frekw]

I actually think being able to separate the measurements from the final calculation/reporting of metrics is the most beautiful and clever part of this change, so I definitely think we should keep the forkDaemon. :)

Also, a single forkDaemon call is likely to pale in comparison to all other fork calls during query execution.

[frekw]

Beautiful optimization (once I managed to figure out how it works :)).

The only thing that caught me is if the .view here actually does anything, since we're directly materializing it to a vector?

[kyri-petrou]

The only thing that caught me is if the .view here actually does anything, since we're directly materializing it to a vector?

@frekw it's a space-complexity optimization rather than a time-complexity one. Without .view we're materializing an intermediate List when we call .reverse, only to then convert it to a Vector. With .view we skip materializing the intermediate List.

[kyri-petrou]

@ghostdogpr are you okay with merging this one in?