-
Notifications
You must be signed in to change notification settings - Fork 14
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
Avoid expensive refreshable creation in a potentially hot path #2274
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,7 +81,9 @@ private static BooleanSupplier shouldSendAcceptGzip(Config cf, Endpoint endpoint | |
// is handled by the client. Note that this will also opt out of response | ||
// compression when the target host resolves to multiple IP addresses. | ||
if (cf.mesh() == MeshMode.DEFAULT_NO_MESH) { | ||
return cf.uris().map(targets -> targets.size() == 1)::get; | ||
// Avoid using refreshable here, as this can be called very frequently, and | ||
// refreshable.map can be expensive. | ||
return () -> cf.uris().get().size() == 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't want to create a refreshable.map target for each endpoint. We could extract a refreshable.map to a higher layer, but this is probably simpler (list.size is just a field read) |
||
} | ||
|
||
return () -> false; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -163,12 +163,6 @@ Builder ticker(Ticker value) { | |
public DialogueChannel build() { | ||
Config cf = builder.build(); | ||
|
||
// Reloading currently forgets channel state (pinned target, channel scores, concurrency limits, etc...) | ||
// In a future change we should attempt to retain this state for channels that are retained between | ||
// updates. | ||
Refreshable<ImmutableList<LimitedChannel>> channels = | ||
cf.uris().map(targetUris -> createHostChannels(cf, targetUris)); | ||
|
||
DialogueClientMetrics clientMetrics = | ||
DialogueClientMetrics.of(cf.clientConf().taggedMetricRegistry()); | ||
|
||
|
@@ -178,16 +172,20 @@ public DialogueChannel build() { | |
.clientType("dialogue-channel-non-reloading") | ||
.build(); | ||
|
||
LimitedChannel nodeSelectionChannel = new SupplierChannel(channels.map(current -> { | ||
// Reloading currently forgets channel state (pinned target, channel scores, concurrency limits, etc...) | ||
// In a future change we should attempt to retain this state for channels that are retained between | ||
// updates. | ||
LimitedChannel nodeSelectionChannel = new SupplierChannel(cf.uris().map(targetUris -> { | ||
reloadMeter.mark(); | ||
log.info( | ||
"Reloaded channel '{}' targets. (uris: {}, numUris: {}, targets: {}, numTargets: {})", | ||
SafeArg.of("channel", cf.channelName()), | ||
UnsafeArg.of("uris", cf.clientConf().uris()), | ||
SafeArg.of("numUris", cf.clientConf().uris().size()), | ||
UnsafeArg.of("targets", current), | ||
SafeArg.of("numTargets", current.size())); | ||
return NodeSelectionStrategyChannel.create(cf, current); | ||
UnsafeArg.of("targets", targetUris), | ||
SafeArg.of("numTargets", targetUris.size())); | ||
ImmutableList<LimitedChannel> targetChannels = createHostChannels(cf, targetUris); | ||
return NodeSelectionStrategyChannel.create(cf, targetChannels); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. combined two refreshable.map calls since we're really doing one thing here, should be easier to follow |
||
})); | ||
|
||
LimitedChannel stickyValidationChannel = new StickyValidationChannel(nodeSelectionChannel); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the most important change, we have a lot of code which attempts to cast Channel to EndpointChannelFactory and use
endpoint(endpoint)
on the result, failing that it will call theChannel.execute(Endpoint,Request)
method, which internally delegates to the preferredendpoint
method. Unfortunately that ends up re-computing a great deal of info on each invocation.This cruft exists to avoid breaking dialogue API :-/