Skip to content

Improve histogram, summary performance under contention by striping observationCount #1794

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.

Sign up for GitHub

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

Open
jack-berg wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Improve histogram, summary performance under contention by striping observationCount #1794

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 24 additions & 7 deletions prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Buffer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
class Buffer {

private static final long bufferActiveBit = 1L << 63;
private final AtomicLong observationCount = new AtomicLong(0);
private final AtomicLong[] stripedObservationCounts;
private double[] observationBuffer = new double[0];
private int bufferPos = 0;
private boolean reset = false;
Expand All @@ -27,8 +27,18 @@ class Buffer {
ReentrantLock runLock = new ReentrantLock();
Condition bufferFilled = appendLock.newCondition();

Buffer() {
stripedObservationCounts = new AtomicLong[Runtime.getRuntime().availableProcessors()];
for (int i = 0; i < stripedObservationCounts.length; i++) {
stripedObservationCounts[i] = new AtomicLong(0);
}
}

boolean append(double value) {
long count = observationCount.incrementAndGet();
AtomicLong observationCountForThread =
stripedObservationCounts[
((int) Thread.currentThread().getId()) % stripedObservationCounts.length];
Copy link
Collaborator Author

@jack-berg jack-berg Jan 21, 2026

Choose a reason for hiding this comment

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

One thing to consider is whether there is a better algorithm for accessing one of these striped instances. With this algorithm based on thread id, its conceivable that the thread ids could be arranged in such a way that there end up being "hot spots" (i.e. AtomicLong instances that end up receiving a disproportionate amount of the updates). LongAdder solves this by using a random number generate to select which "cell" to access.

Copy link
Collaborator Author

@jack-berg jack-berg Jan 26, 2026

Choose a reason for hiding this comment

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

An alternative to using the thread id to strip is to use ThreadLocalRandom.current().nextInt() % stripedObservationCounts.length. Is there any issue with using ThreadLocalRandom here?

Comment on lines +38 to +40
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

Casting thread ID to int before taking modulo can cause issues. Thread.getId() returns a long value, and casting it to int can result in negative values due to overflow, which will cause ArrayIndexOutOfBoundsException when used as an array index after modulo operation with negative dividend. Use Math.abs() on the casted int value or use a different approach like (int) (threadId & Integer.MAX_VALUE) to ensure a non-negative index, or better yet, use Long.hashCode(Thread.currentThread().getId()) which properly handles the conversion.

Suggested change
AtomicLong observationCountForThread =
stripedObservationCounts[
((int) Thread.currentThread().getId()) % stripedObservationCounts.length];
int stripeIndex =
(Long.hashCode(Thread.currentThread().getId()) & Integer.MAX_VALUE)
% stripedObservationCounts.length;
AtomicLong observationCountForThread = stripedObservationCounts[stripeIndex];

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@jack-berg jack-berg Jan 26, 2026

Choose a reason for hiding this comment

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

Yeah yeah should and was planning to fix before merging

long count = observationCountForThread.incrementAndGet();
if ((count & bufferActiveBit) == 0) {
return false; // sign bit not set -> buffer not active.
} else {
Expand Down Expand Up @@ -69,7 +79,10 @@ <T extends DataPointSnapshot> T run(
runLock.lock();
try {
// Signal that the buffer is active.
Long expectedCount = observationCount.getAndAdd(bufferActiveBit);
Long expectedCount = 0L;
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

The variable 'expectedCount' is only assigned values of primitive type and is never 'null', but it is declared with the boxed type 'Long'.

Suggested change
Long expectedCount = 0L;
long expectedCount = 0L;

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@jack-berg jack-berg Jan 26, 2026

Choose a reason for hiding this comment

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

Existing but will fix

for (AtomicLong observationCount : stripedObservationCounts) {
expectedCount += observationCount.getAndAdd(bufferActiveBit);
}

while (!complete.apply(expectedCount)) {
// Wait until all in-flight threads have added their observations to the histogram /
Expand All @@ -81,14 +94,18 @@ <T extends DataPointSnapshot> T run(
result = createResult.get();

// Signal that the buffer is inactive.
int expectedBufferSize;
long expectedBufferSize = 0;
if (reset) {
expectedBufferSize =
(int) ((observationCount.getAndSet(0) & ~bufferActiveBit) - expectedCount);
for (AtomicLong observationCount : stripedObservationCounts) {
expectedBufferSize += (int) (observationCount.getAndSet(0) & ~bufferActiveBit);
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

Unnecessary and potentially lossy cast to int. The observationCount values are being cast to int before being added to expectedBufferSize (which is a long). This cast truncates the value to 32 bits, which could cause incorrect calculations if any individual stripe's count exceeds Integer.MAX_VALUE. Although unlikely in practice due to striping, the cast should be removed to maintain correctness. The masking operation (~bufferActiveBit) already produces the correct count value as a long.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@jack-berg jack-berg Jan 26, 2026

Choose a reason for hiding this comment

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

Existing but will fix

}
reset = false;
} else {
expectedBufferSize = (int) (observationCount.addAndGet(bufferActiveBit) - expectedCount);
for (AtomicLong observationCount : stripedObservationCounts) {
expectedBufferSize += (int) observationCount.addAndGet(bufferActiveBit);
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

Unnecessary and potentially lossy cast to int. The observationCount values are being cast to int before being added to expectedBufferSize (which is a long). This cast truncates the value to 32 bits, which could cause incorrect calculations if any individual stripe's count exceeds Integer.MAX_VALUE. Although unlikely in practice due to striping, the cast should be removed to maintain correctness.

Copilot uses AI. Check for mistakes.
}
}
expectedBufferSize -= expectedCount;

appendLock.lock();
try {
Expand Down
Loading