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

Use StringBuilder instead of indyfied concat #445

Open
punkratz312 opened this issue Jan 14, 2025 · 1 comment
Open

Use StringBuilder instead of indyfied concat #445

punkratz312 opened this issue Jan 14, 2025 · 1 comment
Labels
enhancement New feature or request question Further information is requested recipe

Comments

@punkratz312
Copy link
Contributor

punkratz312 commented Jan 14, 2025

This is done because the latter has a very small
startup overhead (which one can see in a
CPU time flamegraph obtained from a startup
of a REST application)

See:

The proposed change in the code uses StringBuilder instead of the default indyfied string concatenation (introduced in JEP 280). Here's an explanation of why this change is better in this specific context:

  1. Avoids Startup Overhead
    The comment in the code mentions avoiding the "small startup cost incurred by JEP 280." The indyfied concatenation relies on invokedynamic (indy) instructions, which introduce a minimal overhead during startup as the JVM needs to bootstrap the necessary method handles. While this cost is negligible in many cases, avoiding it can be beneficial in performance-sensitive scenarios, especially for applications optimized for fast startup, such as Quarkus.

  2. Predictable Performance
    StringBuilder is a well-established class in Java, known for its consistent and predictable performance when handling string concatenation. It performs concatenation in-place using a mutable character array, avoiding intermediate object creation.
    In contrast, indyfied concatenation dynamically resolves the optimal concatenation strategy at runtime, which may involve creating additional objects or invoking runtime-compilation-like behavior. While efficient in many use cases, it might not always align with the deterministic behavior required for low-level, high-performance code.

  3. Reduced Memory Allocation
    By precomputing the required buffer size with new StringBuilder(length):

The code ensures that no unnecessary array resizing or copying occurs during concatenation.
It reduces temporary object creation compared to the potential internal workings of indyfied concatenation.
4. Better Flamegraph Clarity
The comment also points out that the startup CPU flamegraph reveals the cost of indyfied concatenation. By switching to StringBuilder, developers can reduce this noise in profiling tools, making it easier to identify and address other performance bottlenecks.

Trade-Offs
Code Readability: indyfied concatenation (+) is more readable and concise. However, in low-level code paths or performance-critical areas, the verbosity of StringBuilder is a worthwhile trade-off.
Performance Gains Depend on Context: The performance difference might not be noticeable in all cases but is significant in frameworks like Quarkus that prioritize low-latency startup.
Conclusion
Using StringBuilder in this context ensures predictable performance, eliminates startup overhead, and aligns with the performance goals of Quarkus. This change reflects a deliberate optimization for a specific requirement—fast startup and efficient runtime behavior.

@punkratz312 punkratz312 added the enhancement New feature or request label Jan 14, 2025
@timtebeek timtebeek moved this to Recipes Wanted in OpenRewrite Jan 19, 2025
@timtebeek
Copy link
Contributor

Thanks; adding the examples in the linked PR here

Before

this.mediaTypeString = mediaType.getType() + "/" + mediaType.getSubtype();
this.mediaTypeSubstring = mediaType.getType() + "/*";

After

// we want to avoid the small startup cost incurred by JEP 280 and that shows up in the startup cpu flamegraph
this.mediaTypeString = new StringBuilder(mediaType.getType().length() + 1 + mediaType.getSubtype().length())
        .append(mediaType.getType()).append("/").append(mediaType.getSubtype()).toString();
this.mediaTypeSubstring = new StringBuilder(mediaType.getType().length() + 2).append(mediaType.getType()).append("/*")
        .toString();

Preconditions

As it does hurt readability quite a bit we'd want to be selective where any such recipe gets applied; I'm not sure how to best determine when we'd apply this recipe or not.

Additional context

The stated goal of JEP 280 is outlined as follows:

Change the static String-concatenation bytecode sequence generated by javac to use invokedynamic calls to JDK library functions. This will enable future optimizations of String concatenation without requiring further changes to the bytecode emitted by javac.

Any changes here would then of course mean those future optimizations wont apply if we go for new String Builder instead.

Given the above I wonder if this is even something to write a recipe for, as this type of change is likely only desired where a definite performance issue has been detected, and not something to replicate anywhere we see string concatenation. What are your thoughts here @punkratz312 ?

@timtebeek timtebeek added the question Further information is requested label Jan 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested recipe
Projects
Status: Recipes Wanted
Development

No branches or pull requests

2 participants