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

Optimize memory allocations in StringUtils#cleanPath #26316

Closed
wants to merge 4 commits into from

Conversation

knittl
Copy link
Contributor

@knittl knittl commented Dec 23, 2020

Investigating once again slow Spring Boot start times, I played around with Java Mission Control profiler and found out that StringUtils#cleanPath alone is responsible for 7% of all CPU time spent during setup of the application context. Looking at the implementation I was able to identify some potential hotspots (e.g. LinkedList). The application is currently using Spring Framework 5.1.10 and I was happy to find that some of these issues have already been fixed in more recent versions of Spring.

However there are still a few more opportunities to avoid memory allocations and copying of data:

  • Set a sensible initial capacity for lists and string builders: optimize for the common cases. We will sometimes allocate a few bytes more than required, but at least we avoid re-allocating the backing array. For the StringBuilder we need to iterate the collection twice to calculate the capacity beforehand. But iterating is a lot cheaper compared to allocating memory two or more times.
  • Re-use strings instead of concatenating their parts again: an earlier optimization introduced short-circuiting for a common case. We have computed the return value already before splitting it, so return it directly.
  • Do not concatenate with the empty string: Future JDKs might alleviate this, but at least with Java 11, return "" + somestring; is slower than return somestring;. Obviously it is more expensive memory-wise due to the construction of the newly resulting string.

This PR relates to and builds on #24674, #25650, #25552, #25553, and others.

The list will contain at most pathElements.length elements, and in most
cases exactly this number of elements. Directly set this as initial
capacity.
Introduce an optimized method to join a collection of strings. Iterating
the list is cheaper than resizing (allocate + copy) the backing array of
the StringBuilder used to collect the elements and delimiters.
Optimize the optimization: if the path did not contain any components
that required cleaning, return the (normalized) path as-is. No need to
concatenate the prefix and the trailing path.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 23, 2020
@knittl
Copy link
Contributor Author

knittl commented Aug 30, 2021

Is there still anything required from my part? Should I provide benchmarks to prove the optimization?

Is something else blocking this change?

@bclozel bclozel self-assigned this Aug 30, 2021
bclozel pushed a commit that referenced this pull request Aug 30, 2021
This commit applies several optimizations to StringUtils#cleanPath and
related methods:

* pre-size pathElements deque in StringUtils#cleanPath with
  pathElements.length elements, since this this is the maximum size and
  the most likely case.
* optimize StringUtils#collectionToDelimitedString to calculate the size
  of the resulting String and avoid array auto-resizing in the
  StringBuilder.
* If the path did not contain any components that required cleaning,
  return the (normalized) path as-is. No need to concatenate the prefix
  and the trailing path.

See gh-26316
@bclozel bclozel added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Aug 30, 2021
@bclozel bclozel added this to the 5.3.10 milestone Aug 30, 2021
@bclozel
Copy link
Member

bclozel commented Aug 30, 2021

Closed with cc026fc

@bclozel bclozel closed this Aug 30, 2021
@bclozel
Copy link
Member

bclozel commented Aug 30, 2021

Thanks @knittl - this is now merged.
I ran a few benchmarks and it seems that this optimization helps both StringUtils#delimitedListToStringArray (+40% throughput) and StringUtils#cleanPath (+10% for the worst case).

@knittl
Copy link
Contributor Author

knittl commented Sep 11, 2021

@bclozel thanks for merging, polishing, and benchmarks! I wonder how big the overhead of converting each collection item to a string when pre-computing the string builder size for non-string collections (in cc026fc)? For string collections, it is obviously just a no-op, but for generic collections of a different type stringifying each of its elements twice might incur quite some memory pressure and runtime overhead (depending on the toString complexity)?

One of the reasons the method was duplicated was to have a specialization for string collections. One idea I had was to pre-compute all string representations, copy them to a new collection, then use this collection to populate the string builder. With your JMH benchmarks (thanks!), copying the collection seems to incur quite some overhead itself.

@jhoeller jhoeller added the in: core Issues in core modules (aop, beans, core, context, expression) label Oct 13, 2021
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this pull request Mar 26, 2022
This commit applies several optimizations to StringUtils#cleanPath and
related methods:

* pre-size pathElements deque in StringUtils#cleanPath with
  pathElements.length elements, since this this is the maximum size and
  the most likely case.
* optimize StringUtils#collectionToDelimitedString to calculate the size
  of the resulting String and avoid array auto-resizing in the
  StringBuilder.
* If the path did not contain any components that required cleaning,
  return the (normalized) path as-is. No need to concatenate the prefix
  and the trailing path.

See spring-projectsgh-26316
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants