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

HttpStatus.resolve allocates HttpStatus.values() once per invocation #26842

Closed
richardstartin opened this issue Apr 21, 2021 · 4 comments
Closed

HttpStatus.resolve allocates HttpStatus.values() once per invocation #26842

richardstartin opened this issue Apr 21, 2021 · 4 comments
Assignees
@poutsma
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone
5.3.7

Comments

@richardstartin
Copy link

richardstartin commented Apr 21, 2021 •
edited

I just noticed looking at profiles of some reactor based benchmarks we run to measure Datadog tracer overhead that HttpStatus.resolve allocates an HttpStatus[] once per invocation, so once per response. In a very modest throughput benchmark (~600rps) this is allocating 1MB/s just to resolve the integer status code to the HttpStatus enum value.
Screenshot 2021-04-21 at 22 35 38

I tracked the spring framework code down here.

This could be fixed by caching HttpStatus.values() in an array, which could be iterated over as many times as one likes without any further allocation.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 21, 2021
@poutsma poutsma self-assigned this Apr 22, 2021
@poutsma poutsma added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 22, 2021
@poutsma poutsma added this to the 5.3.7 milestone Apr 22, 2021
@poutsma
Copy link
Contributor

poutsma commented Apr 22, 2021

I think we can do better than a cached array, and might as well start using a Map<Integer, HttpStatus>.

@richardstartin
Copy link
Author

Boxing HTTP status codes can be problematic because the common ones all fall outside the range of the Integer cache. We use this data structure for representing mappings between small, sparse finite sets of integers (such as status codes or ports) and arbitrary things, which works very well for tracing, and something similar may be applicable here.

@poutsma
Copy link
Contributor

poutsma commented Apr 22, 2021 •
edited

Thanks you for help.

I am not sure if we want to introduce a new data structure into the framework to resolve this. I'll make a copy of the array instead, as originally suggested.

@richardstartin
Copy link
Author

That makes sense - we don't record any time spent in HttpStatus.resolve at all, it only shows up for the allocations.

@sync-by-unito sync-by-unito bot mentioned this issue May 12, 2021
Merged
sbrannen added a commit that referenced this issue May 13, 2021
@sbrannen
Fix grammar in inline comment in HttpStatus
fae4848 
See gh-26842
Zoran0104 pushed a commit to Zoran0104/spring-framework that referenced this issue Aug 20, 2021
@poutsma @Zoran0104
Copy HttpStatus::values to prevent allocation
140c616 
Before this commit, HttpStatus::resolve used the values() method in its
logic. This causes a new array to be allocated for each invocation,
and results in memory overhead.

This commit makes a copy of the HttpStatus values array, and uses that
to resolve status codes.

Closes spring-projectsgh-26842
Zoran0104 pushed a commit to Zoran0104/spring-framework that referenced this issue Aug 20, 2021
@sbrannen @Zoran0104
Fix grammar in inline comment in HttpStatus
6bd8111 
See spring-projectsgh-26842
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this issue Mar 26, 2022
@poutsma @lxbzmy
Copy HttpStatus::values to prevent allocation
35e2795 
Before this commit, HttpStatus::resolve used the values() method in its
logic. This causes a new array to be allocated for each invocation,
and results in memory overhead.

This commit makes a copy of the HttpStatus values array, and uses that
to resolve status codes.

Closes spring-projectsgh-26842
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this issue Mar 26, 2022
@sbrannen @lxbzmy
Fix grammar in inline comment in HttpStatus
72e1eeb 
See spring-projectsgh-26842
@poutsma poutsma mentioned this issue May 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@poutsma poutsma

Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Milestone
5.3.7
Development

No branches or pull requests
3 participants
@poutsma @spring-projects-issues @richardstartin