-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Improve average performance of DefaultLongTaskTimer for out-of-order stopping #5591
Improve average performance of DefaultLongTaskTimer for out-of-order stopping #5591
Conversation
@fogninid Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
@fogninid Thank you for signing the Contributor License Agreement! |
aba71f6
to
295d518
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request. What you wrote makes sense. Still, I wanted to verify with some JMH benchmarks so we can put some numbers behind it and have them in place for checking any future changes that would affect performance around this. I made #5595 to add JMH benchmarks.
micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java
Outdated
Show resolved
Hide resolved
Sharing results from my MacBook Pro M1 with the benchmarks in the linked PR with 10,000 active samples and stopping a random sample. As expected, start is slower but the overall time to start and stop on average (with a random sample) is better. Before
After
|
295d518
to
44f147a
Compare
micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR, I really like this.
Not necessarily in this PR but I' also wondering if we should add a JCStress tests on top of #5595.
micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java
Show resolved
Hide resolved
micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java
Outdated
Show resolved
Hide resolved
micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java
Show resolved
Hide resolved
micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java
Outdated
Show resolved
Hide resolved
...eter-core/src/test/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimerTest.java
Outdated
Show resolved
Hide resolved
...eter-core/src/test/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimerTest.java
Outdated
Show resolved
Hide resolved
...eter-core/src/test/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimerTest.java
Outdated
Show resolved
Hide resolved
9396e4d
to
7333777
Compare
Fyi: I polished the PR a bit and also resolved the comments above and rebased it to |
@fogninid Thank you for the contribution! |
The current implementation of DefaultLongTaskTimer optimizes for O(1) task starting, but performs poorly when stopping tasks that are not at the beginning of its internal queue (the oldest ones).
At the worst case, when calling stop immediately after starting, the stop call is currently expected to require O(N) operations.
Depending on the distribution of task lifetimes, the average case would be O(1) only for applications that stop tasks in exactly the same order as they were started; applications completing out-of-order and with unbiased lifetime would experience O(N) average.
Task stopping should not have any intrinsic difference to starting: both action are expected to be performed on application threads, and for a well-functioning application (that is not leaking of piling-up tasks) every call to start is matched by exactly one call to stop.