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

Optimize HttpHeaders.getAcceptLanguageAsLocales #32318

Conversation

kilink
Copy link
Contributor

@kilink kilink commented Feb 23, 2024

The HttpHeaders.getAcceptLanguageAsLocales was incurring overhead from using a Stream, as well as calling the fairly expensive Locale.getDisplayName method.

Switch to using an ArrayList, and skipping over wildcard ranges to avoid needing to check the display name.

Here is a CPU flamegraph from one of our apps where this method call stands out.

image

The HttpHeaders.getAcceptLanguageAsLocales was incurring overhead from using a Stream,
as well as calling the fairly expensive Locale.getDisplayName method.

Switch to using an ArrayList, and skipping over wildcard ranges to avoid needing to check
the display name.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 23, 2024
@jhoeller jhoeller 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 Feb 23, 2024
@jhoeller jhoeller added this to the 6.1.5 milestone Feb 23, 2024
@jhoeller
Copy link
Contributor

@poutsma the present check there seems worth optimizing in the 6.1.x line. getDisplayName being a specific culprit in that method, but also .stream() usage worth reviewing in other HttpHeaders methods.

I'm not sure about the intent of the original getDisplayName check there and whether the range check replacement is 100% equivalent for our purposes, so I'll leave it up to you to refine this if necessary.

@kilink
Copy link
Contributor Author

kilink commented Feb 23, 2024

I'm fairly certain checking of display name was to handle wildcards as described in rfc4647. Based on the validation in Locale.LanguageRange.parse, it appeared to me to be the only possible way to end up with a Locale instance without a display name, as all other invalid language tags are rejected.

Locale.forLanguageTag("*").getDisplayName(); // empty
Locale.forLanguageTag("*-CA").getDisplayName(); // empty

@poutsma poutsma closed this in beb415d Feb 29, 2024
poutsma added a commit that referenced this pull request Feb 29, 2024
poutsma added a commit that referenced this pull request Feb 29, 2024
…sLocales-optimization

* gh-32318:
  Polishing external contribution
  Optimize HttpHeaders.getAcceptLanguageAsLocales
@poutsma
Copy link
Contributor

poutsma commented Feb 29, 2024

Merged. Thanks for submitting a PR, @kilink !

@kilink kilink deleted the http-headers-getAcceptLanguageAsLocales-optimization branch March 15, 2024 20:01
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: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants