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.

Sign up for GitHub

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

Jump to bottom

StringDecoder causes direct allocation of ByteBuffer #29889

Closed
wants to merge 5 commits into from
Closed

StringDecoder causes direct allocation of ByteBuffer #29889

wants to merge 5 commits into from

Conversation

xavier-b
Copy link
Contributor

Since my switch to Spring Boot 3, I have a lot of direct allocations in my metrics.
Indeed, my app use StringDecoder via, for example, @RequestBody String body

In org.springframework.core.codec.StringDecoder, when dataBuffer is backed by an NIO direct buffer, decode method use dataBuffer.toByteBuffer() which will call java.nio.ByteBuffer.allocateDirect(int).

In java.nio.ByteBuffer javadoc we can read :

The buffers returned by this method typically have somewhat higher allocation and deallocation costs than non-direct buffers. [...]
It is therefore recommended that direct buffers be allocated primarily for large,
long-lived buffers that are subject to the underlying system's native I/O operations.
In general it is best to allocate direct buffers only when they yield a measurable
gain in program performance.

Goal of this code in StringDecoder was to decode with a charset, so maybe it's better to use dataBuffer.toString(charset) which doesn't do direct allocation.
I see the same problem in org.springframework.http.codec.FormHttpMessageReader.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 27, 2023
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) in: core Issues in core modules (aop, beans, core, context, expression) labels Jan 27, 2023
@poutsma poutsma self-assigned this Jan 30, 2023
@poutsma poutsma added this to the 6.0.5 milestone Jan 30, 2023
@poutsma poutsma removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 30, 2023
@poutsma poutsma closed this in b7d53ac Jan 30, 2023
poutsma added a commit that referenced this pull request Jan 30, 2023
@poutsma
Merge pull request #29889 from xavier-b:main
c13dfc5 
* gh-29889:
  Use DataBuffer::toString instead of CharBuffer
@poutsma
Copy link
Contributor

poutsma commented Jan 30, 2023 •
edited

Merged, thanks for fixing this!

@poutsma poutsma added the type: enhancement A general enhancement label Jan 30, 2023
@poutsma poutsma mentioned this pull request Feb 7, 2023
Closed
@bclozel bclozel mentioned this pull request Mar 1, 2023
Closed
mdeinum pushed a commit to mdeinum/spring-framework that referenced this pull request Jun 29, 2023
@xavier-b @mdeinum
Use DataBuffer::toString instead of CharBuffer
603e755 
This commit ensures that converting DataBuffers to a String does not
use an expensive ByteBuffer allocation.

Closes spring-projectsgh-29889
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@poutsma poutsma

Labels
in: core Issues in core modules (aop, beans, core, context, expression) in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Milestone
6.0.5
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@xavier-b @poutsma @rstoyanchev @spring-projects-issues