Skip to content

Conversation

BryceYangS
Copy link
Contributor

doDispatch method of Dispatcher servlet class uses hard coded strings, such as "GET" and "HEAD".
so, I replaced it with HttpMethod enum.

doDispatch method of Dispatcher servlet class uses hard coded strings, such as "GET" and "HEAD".
so, I replaced it with HttpMethod enum.
@pivotal-issuemaster
Copy link

@BryceYangS Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 24, 2021
@pivotal-issuemaster
Copy link

@BryceYangS Thank you for signing the Contributor License Agreement!

@TAKETODAY
Copy link
Contributor

i think it's not a good idea

@BryceYangS
Copy link
Contributor Author

BryceYangS commented Apr 29, 2021

i think it's not a good idea

@TAKETODAY Could I hear why you think so? Your feedback will be of great help to my study.

@rstoyanchev rstoyanchev self-assigned this Apr 29, 2021
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 29, 2021
@rstoyanchev rstoyanchev added this to the 5.3.7 milestone Apr 29, 2021
@sbrannen sbrannen changed the title refactor:replace hard coded strings with enum Use HttpMethod enum constants instead of strings in DispatcherServlet Apr 30, 2021
@dreis2211
Copy link
Contributor

dreis2211 commented Apr 30, 2021

I just stumbled over this PR - I hope you don't mind me commenting. I wonder if we should use HttpMethod.GET.name().equals() rather than HttpMethod.GET.matches(). I did a small benchmark (on JDK 11) comparing things, and the former is expectedly a bit faster because it avoids the map access.

MyBenchmark.testMatches                         avgt    6   4,476 ±  1,153   ns/op
MyBenchmark.testNameEquals                      avgt    6   2,462 ±  0,114   ns/op

Given that we deal with DispatcherServlet here, I wonder if we should strive for the better performing option even though it's definitely a micro-optimization.

@jhoeller
Copy link
Contributor

@dreis2211 I suppose we could also fine-tune the HttpMethod.matches implementation to name().equals(method) then - if known to be faster?

@dreis2211
Copy link
Contributor

dreis2211 commented Apr 30, 2021

@jhoeller I was wondering but it actually was like that before - see 3d87718. Maybe you remember why that changed?

@BryceYangS BryceYangS requested a review from rstoyanchev May 1, 2021 03:21
@BryceYangS
Copy link
Contributor Author

@jhoeller @dreis2211 Thank all of your comments. I thought the problem about a performance for a while. If there is a reason why HttpMethod.matches was changed, How about using name().equals(method) in DispatcherServlet?

@TAKETODAY
Copy link
Contributor

@BryceYangS Using name().equals(method) is not as good as using the original

@jhoeller
Copy link
Contributor

jhoeller commented May 3, 2021

No idea why I changed that back then... maybe a vague suspicion that hashing within the Map lookup would be faster than a String equality comparison, or maybe just a stylistic notion? In any case, we can change the current HttpMethod.resolve implementation back to a name comparison if that is known to be faster, and then rely on that implementation where appropriate.

rstoyanchev pushed a commit that referenced this pull request May 4, 2021
@rstoyanchev
Copy link
Contributor

This has been merged and I've also switched HttpMethod matches to String equals comparison with b76e0c4.

@rstoyanchev rstoyanchev closed this May 4, 2021
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this pull request Mar 26, 2022
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) type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants