Avoid double toString call when joining collections

[knittl]

Commit 8d3e8ca optimized how the method
cleanPath normalizes its input path and introduced the method
joinStrings which pre-allocates a StringBuilder of the correct size
to hold the final delimited string.

Commit cc026fc removed this method
again and merged it with collectionToDelimitedString. The merged
implementation however now undoes part of the original optimization:
toString() is called twice on each item of the collection, which will
put quite some pressure on the GC for non-string collections. For string
collections this is basically a no-op because a string simply returns
itself, i.e. this, in its toString implementation.

This commit introduces a specialization for string collections again.
The actual collecting of items into the StringBuilder has now been
extracted and the call sites pass a (potentially pre-allocated)
StringBuilder instance.

This is a followup to PR #26316

[bclozel]

Hello @knittl, could you clarify how you identified this as a performance problem?
This is optimizing for non-string collections, but I'm mostly finding Collection<String> usage in Spring Framework or very cheap toString() for the few other cases. Is this custom code using this method, maybe another Spring project?

With this change and an adapted JMH benchmark, I've found that:

• non-string collection usage has 2x throughput and /2 GC allocation (for a non trivial toString method)
• a -10% throughput and +50% allocation for the common string collection usage

I'm asking this because we tend to only consider optimizations that fit our own usage pattern, especially when it comes with downsides for the main usage pattern or additional maintenance work on our side.

[knittl]

@bclozel sorry for coming back to you only now. TBH, I haven't, I was just hugely annoyed by double-tostring to precompute the stringbuilder size, which seems to negate the initial improvements. The initial code change (#26316) made sure that the public behavior did not change for generic collections and copied a specialization of the method for string-collections.

Unfortunately Java has type erasure so it is not possible to define a method of the same name which accepts a Collection<String> instead of a Collection<?> – that's a shame really!

Another idea that I followed before I came up with the current fix is to always copy the collection to a new string collection (equivalent stream expression: coll.stream().map(Objects::toString).toList()), but that incurred the overhead of copying the collection for the string case too.

The initial attempt (#26316) identified the cleanPath method as a major contributing factor to CPU usage during application start, so I mostly optimized for this case. As you have correctly identified, the "polished" version currently halves throughput and doubles GC for non-string collections when their elements define custom toString implementations.

[bclozel]

No worries @knittl

I understand that the change that got merged doesn't entirely solve the performance problem you've found. Now I still don't know if the issue you've encountered comes from a code path within Spring or if the usage pattern comes from an application. Could you tell us more?

In the first case, we should ensure that this issue is properly addressed one way or another. In the second, we can't really optimize utility methods for a general usage pattern, especially if this comes at a cost for the common use case. As you've found out, we can't really optimize for both here.

[knittl]

@bclozel the initial performance problem was encountered while profiling a Spring (Boot) application during startup. A considerable amount of CPU time is spent in the StringUtils#cleanPath function (which calls/ed the method collectionToDelimitedString internally). My initial attempts only focused on the behavior/performance of cleanPath, without affecting behavior/performance of other utility methods.

Optimizing for both cases (non-string collections with custom, non-cached toString implementations for their elements – and string collections as provided by cleanPath) can only be achieved by duplicating the method under a different name.

See also the JavaDoc of the private method joinStrings in PR #26316:

This is an optimized variant of {@link #collectionToDelimitedString(Collection, String)}, which does not require dynamic resizing of the StringBuilder's backing array.

This new PR (#27557) restores the original (i.e. before #26316) behavior/performance of method collectionToDelimitedString for string collections AND for non-string collections, by initializing the StringBuilder with at least the same capacity as before. Internally, cleanPath now uses a different logic to pre-compute the string's length.

The commit cc026fc unfortunately added overhead for the non-string case (due to the double-toString calls). This overhead was not there before. In a way, that commit has made performance at least twice as bad for non-string collections while optimizing only for the string collection case. Do you know if this method is called more often with strings or non-strings from external code? Perhaps a new method could be introduced (e.g. stringsToDelimitedString), but I'd like to avoid that.

To summarize: this PR aims to restore the original (before Aug 30 2021) performance characteristics of this method, while still providing optimizations when called from cleanPath.

Thanks for your input so far, I am looking forward to hearing your opinion.

[bclozel]

Sorry but I still don't understand the current situation.

1. You're mentioning a "non-String collection use case" but StringUtils#cleanPath is not it, right? Is there a code path in Spring showing this behavior?

2. If we can improve the StringUtils#cleanPath performance, I'd like to get more information about the performance issue you've hit - what's calling this method and what's the usage profile (repeated calls, long strings, etc).

With those two points I can work on JMH benchmarks and get a clearer picture. Thanks!

[knittl]

@bclozel I will find time to run a CPU profile again next week or the week after. Regarding your two points:

1. Yes, you are right. StringUtils#cleanPath passes a string collection. However, the public method collectionToDelimitedString accepts any type of collection. I can imagine other, non-Spring projects to call this method with collections of non-string element types. The current implementation on main performs bad/worse for the non-string calls.
2. I don't see an obvious way to improve cleanPath further. Even with the current implementation, it's not a problem. String#toString is a no-op and implemented simply as return this, so it does not matter if we call it twice. It only matters for custom, non-cached implementations. Spring itself does not enter this code path, but external code very well might.

I will get back to you with a CPU profile in 1 or 2 weeks time. Thanks for your questions

[bclozel]

Sorry @knittl but I'm going to close this PR for the following reason:

I'm asking this because we tend to only consider optimizations that fit our own usage pattern, especially when it comes with downsides for the main usage pattern or additional maintenance work on our side.

Spring Framework *Utils classes are meant as generally useful for Spring libraries and to a lesser extent Spring applications. We can't spend time optimizing for use cases outside of that scope, especially if this decreases the performance of the main one.

In this case, I think you should consider using a custom method that better fits your use case.

[knittl]

@bclozel I accept that, but I don't understand your point about "decreasing the performance of the main one". Performance of cleanPath remains unaffected by this change and performance of collectionToDelimitedString is no worse than pre-August (for all cases. For some cases it might even be a tiny bit better due to a larger initial capacity of the string builder).

To repeat: I don't have an active use case which calls this method. It's just that the signature of this method is confusing as it allows collections of any element type to be passed, while intentionally providing an inefficient implementation for non-string inputs (that is: worse performance since Aug 2021). Maybe this method should have only accepted Collection<String> inputs in the first place? (Or a new method stringsToCsv which only allows string-collection inputs?) Ideally, this method should at least document that it handles string collections well and non-string collections … not so well.

Lots of other applications directly use Spring's StringUtils in a similar manner to Apache Commons. A quick GitHub search turns up 33k calls to StringUtils#collectionToDelimitedString (not necessarily from the Spring class). Not sure how many of them pass string-only collections.

Thanks for sharing your insights! :)

[bclozel]

Sorry you're right, this decision is not about "decreasing the performance of the main one", but rather choosing not to add utility code or optimize for use cases we're not responsible of.

To repeat: I don't have an active use case which calls this method.

That wasn't clear to me in the original PR nor this one; knowing that would have been a good reason in itself to cut this discussion short. There are many places that could use performance optimizations in Spring Framework, we'd rather focus on the ones that have a meaningful impact on most Spring applications.

Thanks!