Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

RemoteControlledSampler.sample() is a bottleneck #608

Closed
yborovikov opened this issue Mar 21, 2019 · 1 comment · Fixed by #609
Closed

RemoteControlledSampler.sample() is a bottleneck #608

yborovikov opened this issue Mar 21, 2019 · 1 comment · Fixed by #609

Comments

@yborovikov
Copy link
Contributor

Requirement

Tracing should not be a bottleneck in multithreaded apps.

Problem - what in Jaeger blocks you from solving the requirement?

RemoteControlledSampler.sample() is synchronized.

This is causing a major slowdown in multithreaded applications:

sample

Proposal - what do you suggest to solve the problem or improve the existing situation?

  1. Remove unneeded synchronization.
  2. Synchronize on non-public objects (instead of this or .class) where synchronization is unavoidable.
  3. Clean up .equals() - avoid synchronized blocks (and remove the read locks?)
  4. Explain the need for every remaining synchronization (in the code comments).

Any open questions to address

What is the purpose of RemoteControlledSampler.getLock()? Some JavaDoc would be helpful there.

@jpkrohling
Copy link
Collaborator

Based on the package name, looks like you are using an older version of the client. But I expect the same to happen with the latest versions, as this hasn't changed much, if at all.

In any case: we are planning on doing some performance measurements in the next couple of months with @gsoria and would appreciate any inputs you might have.

The first step is certainly to come up with a set of JMH scenarios representing the real-world usage of the client. The one that you described should certainly be part of it, so, if you are able to build a JMH test already, this would be highly appreciated.

Once we have that, we'll be able to measure the performance and assess the impact of the improvements.

pavolloffay pushed a commit that referenced this issue Apr 29, 2019
* Fix #608 - multithreaded performance/synchronization issues

This removes synchronization from RemoteControlledSampler
and makes RateLimiter thread-safe.

Signed-off-by: Yegor Borovikov <yegor@uber.com>

* Revert RemoteControlledSamplerTest changes

Signed-off-by: Yegor Borovikov <yegor@uber.com>

* Further optimize RateLimiter.checkCredit()

Signed-off-by: Yegor Borovikov <yegor@uber.com>

* Move clock.currentNanoTicks() inside optimistic retry loop

Signed-off-by: Yegor Borovikov <yegor@uber.com>

* Log update failure; add timer health test

Signed-off-by: Yegor Borovikov <yegor@uber.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants