Cache parsed URIs throughout Dialogue #2432
Conversation
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.
Generate changelog in
|
| * This prefix may reconfigure several aspects of the client to work better in a world where requests are routed | ||
| * through a service mesh like istio/envoy. | ||
| */ | ||
| private static final String MESH_PREFIX = "mesh-"; |
There was a problem hiding this comment.
At this point the mesh- prefix is no longer used (or supported) in any environment -- it should make things simpler if we introduce another change first to remove support
There was a problem hiding this comment.
I took a look at removing mesh mode, and it has tentacles across a bunch of modules & tests, so might hold off on removing that until after this PR if that's ok.
| @@ -100,8 +102,9 @@ final class ApacheHttpClientBlockingChannel implements BlockingChannel { | |||
| public Response execute(Endpoint endpoint, Request request) throws IOException { | |||
| // Create base request given the URL | |||
| URL target = baseUrl.render(endpoint, request); | |||
| URI targetUri = Uris.tryParse(target.toString()).uriOrThrow(); | |||
There was a problem hiding this comment.
Unlike the base URIs, this value will have much higher cardinality any time an endpoint includes path parameters. I'm a little bit uneasy about caching.
We could bypass the URL and URI types entirely, setting specific elements directly on the request builder instead. That would require expanding the BaseUrl API a bit, but may produce better results. What do you think?
There was a problem hiding this comment.
good call, updated to just use the URL protocol, authority, and file which avoids some of the more expensive new URI(String) parsing and not flooding the URI parsing cache with high cardinality URIs
There was a problem hiding this comment.
split this portion out to a separate PR #2437 as this is portion is the biggest win by avoiding a URI parse per request.
| } catch (final URISyntaxException ex) { | ||
| throw new HttpException("Cannot convert host to URI: " + target, ex); | ||
| } | ||
| final URI targetUri = parseTargetUri(target); |
There was a problem hiding this comment.
I suspect the cardinality is similarly high here, however unlike the per-request parse, this should only occur once per new connection we create, which should be an order or two lower magnitude than requests.
My preference is not to use the cache here. I suspect the perf cost of uri parsing is insignificant compared to tls handshake overhead. If that turns out not to be the case, It think we could plumb through the original base uri via the HttpContext so we can reuse the same value for each connection.
Before this PR
Noticed
new URI(String)and some of its side effects in JFRs from services making high volumes of Dialogue requests.After this PR
==COMMIT_MSG==
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.
==COMMIT_MSG==
Possible downsides?