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

Reduce String allocations in TransactionAspectSupport.methodIdentification() [SPR-14760] #19326

Closed
spring-projects-issues opened this issue Sep 28, 2016 · 5 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Sep 28, 2016

Christoph Dreis opened SPR-14760 and commented

Hey,

in some of my recent tests I noticed a rather "high" pressure on the heap coming from TransactionAspectSupport.methodIdentification(). (The image below shows the result of a 10 minute flight recording) for StringBuilder.append(String). Of course there are some more MBs (~200) allocated for the StringBuilder and the actual toString() calls - in case you need this data as well - just tell me.

!transaction-methodIdentification.jpg|thumbnail!

As far as I understood the method it's "just" used for logging and monitoring - both of which are disabled in most situations, I'd say. Is this observation correct? Unfortunately, I didn't figure out yet how to implement my own version of TransactionAspectSupport inside a Spring-Boot app in order to workaround the problem at least for us, but that might be an issue for Boot after all.

Nonetheless, I attached a small PR that should at least force the compiler to concat the dot with a char instead of a String. Maybe this is the first step for 4.x until you can maybe come up with something smarter in 5.x.

Cheers,
Christoph


Affects: 4.3.3

Attachments:

Issue Links:

  • #21035 @Transactional annotation lead to a huge memory allocation during creation String representation of Method uses for logging only ("is duplicated by")
  • #19202 Consistent comma splitting without regex overhead (e.g. in MediaType/MimeType)

Referenced from: pull request #1186

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 16, 2016

Christoph Dreis commented

Any chance the intermediate step provided in the PR makes it into 4.3.4? Would highly appreciate it.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 16, 2016

Juergen Hoeller commented

I did a rather extensive pass across the codebase, changing all concatenations of single-char separators to single quotes. This certainly makes sense stylistically. However, I wonder whether this really makes a noticeable difference at runtime? Do you have actual profiling results for this?

In any case, for that method identification specifically, I've added a descriptor field to our DefaultTransactionAttribute object which we're caching per method already, so we're only building that method identification once now. Also, all such code across the codebase delegates to a common ClassUtils.getQualifiedMethodName(Method, Class) utility now. To be pushed tomorrow.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 16, 2016

Christoph Dreis commented

I did again some 10 minute flight recordings today on our real application and noticed that there is - depending on how many transactions there are - up to 4-5GB pressure on the heap from the complete method. So thank you for resolving this.

Unfortunately, I just have a synthetic benchmark for you this evening to respond to your question if it's worthwile to switch from String to single-char concats.

Isolated string allocation test

||Benchmark||Mode||Cnt||Score||Error||Units|
|MyBenchmark.testChar|thrpt|20|11990894,558|± 111560,793|ops/s|
|MyBenchmark.testString|thrpt|20|10910108,066|± 117409,915|ops/s|

While this is roughly a ~10% improvement in terms of throughput, I guess in the real world this will be barely noticeable for most applications - unless it's a really hot path or inside a synchronized block that could benefit from being faster. But after all it's execution time that could be spent somewhere else. In the end it could also be benefitial in terms of garbage collections, that one might save. If you're somehow memory restricted - like we are - you try to save everything you can ;-)

I additionally did a synthetic test that should mimic the new behaviour of the DefaultTransactionAttribute - e.g. caching this information instead of creating the string on every method call.

Caching allocated strings

||Benchmark||Mode||Cnt||Score||Error||Units|
|MyBenchmark.testNormal|thrpt|20|10978627,067 |± 74568,315|ops/s|
|MyBenchmark.testCached|thrpt|20|352844033,692|± 4690867,680|ops/s|

This shows a factor of around 32, which for transaction heavy applications should make a difference.

Anyhow - I highly appreciate the effort on the complete codebase and more importantly on the initial subject. Thanks again!

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 20, 2016

Christoph Dreis commented

I'd be happy to provide profiling results for the current snapshot in order to show the results. Is the change already available somewhere?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 21, 2016

Juergen Hoeller commented

Along with several other refinements, this is available in master now and will also show up in the upcoming 4.3.4.BUILD-SNAPSHOT.

Thanks for all the profiling :-)

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.