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

.sample() methods of PerOperationSampler and GuaranteedThroughputSampler are synchronized #807

Closed
william-tran opened this issue Oct 13, 2021 · 1 comment
Labels

Comments

@william-tran
Copy link

william-tran commented Oct 13, 2021

Along the lines of #608 and #609, with the move to Java 8 baseline in #802, we have further opportunities to improve sampler concurrency, and we should eliminate synchronized on all implementations of Sampler.sample(). Contention remains on all calls to RemoteControlledSampler.sample() due to the underlying synchronization of PerOperationSampler.sample().

@william-tran
Copy link
Author

I have the changes in an internal fork awaiting approval for contribution upstream.

william-tran pushed a commit to autonomic-ai/jaeger-client-java that referenced this issue Oct 14, 2021
- Add volatile keyword to underlying samplers so updated references can
be made available to other threads.

See jaegertracing#609 for
similar work.

Fixes jaegertracing#807

Signed-off-by: Will Tran <[email protected]>
william-tran pushed a commit to autonomic-ai/jaeger-client-java that referenced this issue Oct 14, 2021
- Use a ConcurrentHashMap for operationNameToSampler so that any number
of concurrent calls to sample can be made and safely modify it.
- Add volatile to any fields that can be changed by the update method to
ensure visibility of changes to other threads.
- Retain instances of GuaranteedThroughputSampler to preserve their rate
limit balances across updates when parameters don't change improving on
jaegertracing/jaeger#1729

See jaegertracing#609 for
similar work.

Fixes jaegertracing#807

Signed-off-by: Will Tran <[email protected]>
@william-tran william-tran reopened this Oct 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants