Skip to content

Estimate StringBuilder capacity for exported names #919

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

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

Conversation

nicktelford
Copy link
Contributor

When we generate metric names, we use the default StringBuilder
constructor, which allocates a 16 character buffer.

Most metrics will exceed this, especially when we start adding
attributes to it.

This causes StringBuilder to "grow", automatically, by allocating a
new array and copying its contents over to it. Each time it only grows
enough to contain the appended String, which means we need to grow on
almost every append call.

While allocating new memory is cheap in the JVM, copying memory is not.
These copies add up, especially with a large number of long-named
metrics with many attributes.

Instead, we can calculate the necessary capacity of the StringBuilder
up-front, which should avoid doing any copying during append.

Signed-off-by: Nick Telford [email protected]

When we generate metric names, we use the default `StringBuilder`
constructor, which allocates a _16 character_ buffer.

Most metrics will exceed this, especially when we start adding
attributes to it.

This causes `StringBuilder` to "grow", automatically, by allocating a
new array and copying its contents over to it. Each time it only grows
enough to contain the `append`ed String, which means we need to grow on
almost every `append` call.

While allocating new memory is cheap in the JVM, copying memory is not.
These copies add up, especially with a large number of long-named
metrics with many attributes.

Instead, we can calculate the necessary capacity of the `StringBuilder`
up-front, which should avoid doing any copying during `append`.

Signed-off-by: Nick Telford <[email protected]>
@nicktelford
Copy link
Contributor Author

nicktelford commented Feb 22, 2024

See the attached flame graph, which demonstrates the memory being allocated and copied by StringBuilder.append() under recordBean.

Note: I'm pretty sure the JVM is inlining Receiver#defaultExport into JmxCollector#recordBean, which is why defaultExport is missing from the stack.

image

@nicktelford
Copy link
Contributor Author

I'm no longer convinced that the lack of estimating StringBuilder size is the main reason for all these copies. When StringBuilder expands, it always at least doubles its current size, which should result in relatively minimal copies in this particular case.

I think the primary cause of the flame graph above is actually simply poor optimizations of String concatenation in code compiled under Java 8. See #920 for more details.

Since computing the exact size up-front requires iterating the attributes, it might not actually be much more efficient to do this.

However, since copying memory is generally less efficient than iterating a linked list, I'm going to leave this PR open, as it will probably provide a very modest performance improvement.

@DSmithVA
Copy link

may be faster overall to just set an initial capacity on all the StringBuilder instances like 256 bytes - these are so short-lived that with escape analysis, they may be fully stack-allocated and never touch the heap.

and yes @nicktelford a lot of this is from naive String concatenation as noted in #920 - i tried replacing all String concatenation with StringBuilder calls and got this reduced, however it was a bit messy. Best outcome was a shared StringBuilder, reset per-use via setLength(0).

@dhoard
Copy link
Collaborator

dhoard commented May 28, 2025

@DSmithVA are your code changes available? Thoughts on using a ThreadLocal StringBuilder?

public final class ReusableStringBuilder {

    // Constructor 
    private ReusableStringBuilder() {
        //INTENTIONALLY BLANK
    }

    // Thread-local holder for a reusable StringBuilder
    private static final ThreadLocal<StringBuilder> THREAD_LOCAL_STRING_BUILDER =
            ThreadLocal.withInitial(() -> new StringBuilder(1024)); // Optional initial capacity

    /**
     * Returns a thread-local, reusable StringBuilder with its length reset to 0.
     *
     * @return a cleared StringBuilder instance
     */
    public static StringBuilder acquire() {
        StringBuilder sb = THREAD_LOCAL_STRING_BUILDER.get();
        sb.setLength(0); // Clear for reuse
        return sb;
    }
    
    /**
     * Releases the current thread's reference to the reusable StringBuilder.
     * Should be used in thread pool environments to prevent memory leaks.
     */
    public static void release() {
        THREAD_LOCAL_STRING_BUILDER.remove();
    }
}

We could then acquire/release it in the JmxCollector collect() method

@DSmithVA
Copy link

I don't have the StringBuilder changeset anymore, it was fairly noisy/intrusive for something that could've just been fixed by a newer java version. By far, a bigger improvement was to cache MBeans between runs and update the cache via NotificationListener (special case for DynamicMBean impls), but the changes kept getting more involved, to the point where a rewrite was a lot less work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants