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

Do not acquire lock when registering metrics on the happy path #15378

Conversation

@cangencer
Copy link
Contributor

commented Jul 26, 2019

Previously two calls to CHM and a lock was required to register a metric.
The lock should only be necessary when there is contention on the same metric, so
do not acquire it on the happy path.

Previously two calls to CHM and a lock was required to register a metric.
The lock should only be necessary when there is contention on the same metric, so
do not acquire it on the happy path.
probeInstance.source = source;
probeInstance.function = function;
if (logger.isFinestEnabled()) {
logger.finest("Registered probeInstance " + name);

This comment has been minimized.

Copy link
@blazember

blazember Jul 26, 2019

Contributor

Probably this may be logging falsely and logOverwrite too. If the object is taken from putIfAbsent gets removed concurrently, registration/overwrite doesn't happen. This behavior looks right, but the logging may be misleading. Checking contains in the critical section and logging only in that case might solve this.

Note: this code is inherently racy and we are about to fix it in 4.0 by changing explicit registration/deregistration to dynamic discovery.

This comment has been minimized.

Copy link
@pveentjer

pveentjer Jul 29, 2019

Member

It that still planned for 4.0?

This comment has been minimized.

Copy link
@cangencer

cangencer Jul 29, 2019

Author Contributor

@blazember You are right I think, you might end up with a missing probe instance in this case, if the it's removed right after the putIfAbsent call. Should the probe be reinserted to the map instead of modifying the source/function? Is there a reason for modifying the existing registered one instead of replacing it?

This comment has been minimized.

Copy link
@blazember

blazember Jul 29, 2019

Contributor

@cangencer We can add it back if we detect that the probe was removed since putIfAbsent, but in that case, we might leak. The replaced code may also leak or lose probe. I think there is no good solution with the code in 3.12.x. The PR proposes a solution that's in line with the replaced code. We use putIfAbsent to reason about the order of the execution of the concurrent threads, and if the order is register-than-deregister, we won't have the probe registered, just like with the replaced code where we serialized the threads on the lock. I would fix only the logging.

@pveentjer Yes, solving the lifecycle issues is a requirement of the metrics work for 4.0.

Copy link
Contributor

left a comment

LGTM with a minor suggestion regarding logging.

@pveentjer

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

Can you also make a forward port for 4.0?

@cangencer cangencer merged commit 91580f1 into hazelcast:maintenance-3.x Jul 30, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@cangencer cangencer deleted the cangencer:3.x-maintenance/optimize-metrics branch Jul 30, 2019
@cangencer

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

@pveentjer as this is being redone in 4.0, will not do a forward port for now.

Holmistr added a commit that referenced this pull request Sep 4, 2019
Previously two calls to CHM and a lock was required to register a metric.
The lock should only be necessary when there is contention on the same metric, so
do not acquire it on the happy path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.