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

Fix assertions in StompHeaderAccessor [SPR-14625] #19192

Closed
spring-projects-issues opened this issue Aug 24, 2016 · 5 comments
Closed

Fix assertions in StompHeaderAccessor [SPR-14625] #19192

spring-projects-issues opened this issue Aug 24, 2016 · 5 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Aug 24, 2016

Christoph Dreis opened SPR-14625 and commented

Hey,

while looking into #19191 I noticed another case of unnecessary and rather costly string concatenations. Not as severe as in #19191 but with a factor of 8 it still has an effect. Applications that send a lot of websocket messages could benefit from this as it eases the pressure on the heap.

Benchmark Mode Cnt Score Error Units
MyBenchmark.testAssertNormal thrpt 30 21121,046 225,696 ops/s
MyBenchmark.testAssertEnhanced thrpt 30 173947,320 2253,248 ops/s

I used traditional if statements instead of the supplier functionality in Assert in order to be able to backport it to 4.3.x or even 4.2.x - much like the fixes for #19191.

Cheers


Affects: 4.2.7, 4.3.2

Reference URL: #1139

Issue Links:

Backported to: 4.2.8

@spring-projects-issues
Copy link
Collaborator Author

Sam Brannen commented

FYI: on Spring 5.0, the proposed changes can be rewritten as lambda expressions as follows.

Assert.state(SimpMessageType.MESSAGE.equals(getMessageType()), () -> "Unexpected message type " + getMessage());

Assert.state(SimpMessageType.MESSAGE.equals(getMessageType()), () -> "Unexpected message type " + getMessage());

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Aug 24, 2016

Christoph Dreis commented

I know - while working on #19191 I did it like this. But as it affects previous versions, I wanted to ease the backport process.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Aug 24, 2016

Sam Brannen commented

Understood!

I hadn't seen #19191. So apologies for that.

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

Good catch again... Those assertions in StompHeaderAccessor even included getMessage() instead of getMessageType(), and they unnecessarily use equals over an identity comparison for an enum value.

A further assertion in that class, in appendPayload, once again uses the wrong exception type and doesn't give any exception message at all. Fixed as well now.

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

Last but not least, thanks for the detailed breakdown in the benchmark numbers! This is really useful insight.

@spring-projects-issues spring-projects-issues added type: bug A general bug status: backported An issue that has been backported to maintenance branches in: web Issues in web modules (web, webmvc, webflux, websocket) labels Jan 11, 2019
@spring-projects-issues spring-projects-issues added this to the 4.3.3 milestone Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Development

No branches or pull requests

2 participants