Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize assembly tracing #1466

Merged
merged 15 commits into from
Jan 3, 2019
Merged

Optimize assembly tracing #1466

merged 15 commits into from
Jan 3, 2019

Conversation

bsideup
Copy link
Contributor

@bsideup bsideup commented Dec 14, 2018

On Java 8, getStackTraceDepth + getStackTraceElement are much faster than new Exception().getStackTrace().
There are no such methods in Java 9, but StackWalker API, which is a bit slower than native ones, but also outperforms getStackTrace().

If I run the provided benchmark on my machine (Mac OS X, 2,2 GHz Intel Core i7), I get the following numbers:
Reactor core 3.2.3.RELEASE:

Benchmark                                  (stackSize)  Mode  Cnt       Score       Error  Units
AssemblyTraceBenchmark.withTracingOnJDK11           10  avgt    5  296039.399 ± 27838.411  ns/op
AssemblyTraceBenchmark.withTracingOnJDK11           40  avgt    5  513306.643 ± 11894.858  ns/op
AssemblyTraceBenchmark.withTracingOnJDK11           80  avgt    5  800795.784 ± 48429.266  ns/op
AssemblyTraceBenchmark.withTracingOnJDK8            10  avgt    5  203167.404 ±  2892.552  ns/op
AssemblyTraceBenchmark.withTracingOnJDK8            40  avgt    5  385853.825 ± 12205.563  ns/op
AssemblyTraceBenchmark.withTracingOnJDK8            80  avgt    5  617802.999 ± 12685.994  ns/op
AssemblyTraceBenchmark.withoutTracing               10  avgt    5    3180,662 ±   187,830  ns/op
AssemblyTraceBenchmark.withoutTracing               40  avgt    5    4745,460 ±   121,963  ns/op
AssemblyTraceBenchmark.withoutTracing               80  avgt    5    6190,435 ±   431,500  ns/op

This branch:

Benchmark                                  (stackSize)  Mode  Cnt      Score      Error  Units
AssemblyTraceBenchmark.withTracingOnJDK11           10  avgt    5  48189,066 ± 1246,476  ns/op
AssemblyTraceBenchmark.withTracingOnJDK11           40  avgt    5  50204,746 ± 2709,962  ns/op
AssemblyTraceBenchmark.withTracingOnJDK11           80  avgt    5  51761,306 ±  663,210  ns/op
AssemblyTraceBenchmark.withTracingOnJDK8            10  avgt    5  36983,888 ± 2164,248  ns/op
AssemblyTraceBenchmark.withTracingOnJDK8            40  avgt    5  46101,357 ± 1066,614  ns/op
AssemblyTraceBenchmark.withTracingOnJDK8            80  avgt    5  60321,146 ± 2099,422  ns/op
AssemblyTraceBenchmark.withoutTracing               10  avgt    5   3052,667 ±   56,492  ns/op
AssemblyTraceBenchmark.withoutTracing               40  avgt    5   4629,085 ±  181,765  ns/op
AssemblyTraceBenchmark.withoutTracing               80  avgt    5   6045,648 ±  169,674  ns/op

As you can see, it is 4-10x times faster and much less dependant on the stack size. Also, the error is lower.

Open questions

On Java 8, `getStackTraceDepth` + `getStackTraceElement` are much
faster than `new Exception().getStackTrace()`.
There are no such methods in Java 9, but `StackWalker` API, which is
a bit slower than native ones, but also outperforms `getStackTrace()`.
@bsideup bsideup added the status/need-decision This needs a decision from the team label Dec 14, 2018
@bsideup bsideup added the wip label Dec 14, 2018
@codecov-io
Copy link

codecov-io commented Dec 14, 2018

Codecov Report

Merging #1466 into master will increase coverage by 0.2%.
The diff coverage is 78.88%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #1466     +/-   ##
===========================================
+ Coverage     84.27%   84.48%   +0.2%     
- Complexity     3895     4216    +321     
===========================================
  Files           358      359      +1     
  Lines         29435    30528   +1093     
  Branches       5464     5671    +207     
===========================================
+ Hits          24806    25791    +985     
- Misses         3030     3105     +75     
- Partials       1599     1632     +33
Impacted Files Coverage Δ Complexity Δ
...in/java/reactor/core/publisher/MonoOnAssembly.java 92.3% <100%> (-2.14%) 7 <0> (-2)
...reactor/core/publisher/FluxCallableOnAssembly.java 80% <100%> (ø) 6 <0> (ø) ⬇️
...ore/src/main/java/reactor/core/publisher/Flux.java 98.97% <100%> (+0.23%) 711 <2> (+201) ⬆️
...re/src/main/java/reactor/core/publisher/Hooks.java 96.31% <100%> (+1.08%) 41 <7> (+6) ⬆️
...in/java/reactor/core/publisher/FluxOnAssembly.java 91.76% <100%> (+0.85%) 16 <0> (-4) ⬇️
...ore/src/main/java/reactor/core/publisher/Mono.java 94.76% <100%> (+0.95%) 354 <2> (+92) ⬆️
...reactor/core/publisher/MonoCallableOnAssembly.java 72.22% <100%> (ø) 8 <0> (ø) ⬇️
...main/java/reactor/core/publisher/SignalLogger.java 74.45% <100%> (ø) 70 <0> (ø) ⬇️
...ctor/core/publisher/ConnectableFluxOnAssembly.java 85.71% <100%> (ø) 8 <1> (ø) ⬇️
...reactor/core/publisher/ParallelFluxOnAssembly.java 69.23% <100%> (-8.55%) 10 <1> (-3)
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f9e176...07a2787. Read the comment docs.

@bsideup bsideup removed status/need-decision This needs a decision from the team wip labels Dec 18, 2018
@bsideup bsideup mentioned this pull request Dec 18, 2018
* @author Simon Baslé
* @author Sergei Egorov
*/
final class Tracer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not Tracer to keep that naming space for tracing (aka zipkin) features

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the class, not needed anymore. The content moved to Traces where it belongs :)

@bsideup bsideup added type/enhancement A general enhancement area/java9 This belongs to the java9 compatibility theme labels Jan 2, 2019
@bsideup bsideup merged commit ebd7113 into master Jan 3, 2019
@bsideup bsideup deleted the optimize_assembly_tracing branch January 3, 2019 17:36
@bsideup bsideup added this to the 3.2.4.RELEASE milestone Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/java9 This belongs to the java9 compatibility theme type/enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants