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

Make updateGauge lock-free in critical paths of APC and APCng #135

Merged
merged 4 commits into from
Nov 24, 2023

Conversation

TobiasBengtsson
Copy link
Contributor

See commit messages for more info. TL;DR setting values on a gauge can cause high contention due to multiple APCu write locks.

When setting a value on a gauge, first apcu_store is called in
updateGauge and then apcu_add is called in storeMetadata. Both of
these operations takes a write lock in APCu which can cause contention.

- Change updateGauge to use a compare-and-swap algorithm instead of
apcu_store, unless we're adding something for the first time(s) in
which case we just set it with apcu_store.
- Don't call updateMetadata and storeLabelKeys unless it's the first
time we're setting a gauge (matches the behaviour of inc/dec).
- For extra safety, make a cheap rlock-only check in storeMetadata if
the key exists before calling apcu_add with a wlock.

Signed-off-by: Tobias Bengtsson <me@tobiasbengtsson.se>
When setting a value on a gauge, apcu_store is always called two
times, taking a write lock each that can cause contention.

- Replace apcu_store with compare-and-swap algorithm unless we store
for the first time(s)
- Eagerly fetch old value instead of apcu_exists, since we will anyway
need it for the compare-and-swap algorithms that follows, saving one
call to APCu
- Make sure we never enter an infinite loop in the compare-and-swap
section by falling back to a first-time insert if $old === false. A
fix for the potential infinite loop below will be done in a separate
commit.

Signed-off-by: Tobias Bengtsson <me@tobiasbengtsson.se>
Copy link
Member

@LKaemmerling LKaemmerling left a comment

Choose a reason for hiding this comment

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

LGTM; thank you!

@LKaemmerling LKaemmerling merged commit c7c174b into PromPHP:main Nov 24, 2023
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants