Optimize target selection and URI parsing #2398
Merged
+225
−61
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Generate changelog in
|
schlosna
force-pushed
the
davids/dns-uri
branch
2 times, most recently
from
81330da to
899e9e6
Compare
carterkozak
reviewed
Oct 28, 2024
Comment on lines
200
to
202
| TargetUri.Builder builder = TargetUri.builder().uri(uri); | ||
| targetUris.addAll(Collections2.transform(resolvedAddresses, addr -> builder.resolvedAddress(addr) | ||
| .build())); |
There was a problem hiding this comment.
I'm not a big fan of the partial-builder-as-factory pattern, since sometimes folks add protections later on to prevent interactions after build() is called. It shouldn't be an issue here since. the TargetUri type is defined within this repo, so I don't have a terribly strong preference
There was a problem hiding this comment.
Perhaps we add an efficient TargetUri.of(uri, address) factory method to use instead? That way we bypass the builder entirely.
There was a problem hiding this comment.
good call, updated
carterkozak
reviewed
Oct 28, 2024
| .register(stats -> Caffeine.newBuilder() | ||
| .maximumWeight(100_000) | ||
| .<String, Parsed>weigher((key, _value) -> key.length()) | ||
| .expireAfterAccess(Duration.ofMinutes(1)) |
schlosna
force-pushed
the
davids/dns-uri
branch
2 times, most recently
from
536c6bf to
83c3c0d
Compare
|
I'm not sure why that check is failing on this PR, but I suspect merging this will fix it: #2403 |
|
#2403 has merged, could you please rebase this to develop when you have a moment? |
schlosna
force-pushed
the
davids/dns-uri
branch
from
83c3c0d to
668b607
Compare
|
Released 4.3.0 |
schlosna
added a commit
that referenced
this pull request
Nov 22, 2024
Parsing String to URI can be expensive in terms of CPU and allocations for high throughput services. This generalizes the caching previously added to DnsSupport in #2398 to also cover ApacheHttpClientBlockingChannel requests and HttpsProxyDefaultRoutePlanner proxy parsing to leverage a shared parsed URI cache.
schlosna
added a commit
that referenced
this pull request
Nov 22, 2024
Parsing String to URI can be expensive in terms of CPU and allocations for high throughput services. This generalizes the caching previously added to DnsSupport in #2398 to also cover ApacheHttpClientBlockingChannel requests and HttpsProxyDefaultRoutePlanner proxy parsing to leverage a shared parsed URI cache.
schlosna
added a commit
that referenced
this pull request
Jan 26, 2025
Parsing String to URI can be expensive in terms of CPU and allocations for high throughput services. This generalizes the caching previously added to DnsSupport in #2398 to also cover ApacheHttpClientBlockingChannel requests and HttpsProxyDefaultRoutePlanner proxy parsing to leverage a shared parsed URI cache.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this PR
While reviewing some JFRs from dialogue client heavy service, notice that there are a lot of
byte[]allocations as a result of parsingStringtoURI. In most cases, these will be be the same input string, so are good candidates for caching.After this PR
==COMMIT_MSG==
Optimize target selection and URI parsing
==COMMIT_MSG==
Possible downsides?
We don't have a great way to inject cache sizing/weight configuration or tagged metrics, so for now have hardcoded max cache weight.