From 81b856b5d37791be779ccee8f64c77500d5cd228 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Sat, 30 Oct 2021 11:48:22 +0800 Subject: [PATCH 1/8] Remote: Limit max number of TCP connections to gRPC server by --remote_max_connections. --- .../lib/remote/ReferenceCountedChannel.java | 10 +++-- .../build/lib/remote/RemoteModule.java | 9 +++-- .../remote/grpc/DynamicConnectionPool.java | 39 +++++++++++++------ .../lib/remote/options/RemoteOptions.java | 8 ++-- 4 files changed, 44 insertions(+), 22 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/ReferenceCountedChannel.java b/src/main/java/com/google/devtools/build/lib/remote/ReferenceCountedChannel.java index 5c5af7e013400a..8a68e2dbc39550 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ReferenceCountedChannel.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ReferenceCountedChannel.java @@ -62,8 +62,12 @@ public ReferenceCounted touch(Object o) { private final AtomicReference authorityRef = new AtomicReference<>(); public ReferenceCountedChannel(ChannelConnectionFactory connectionFactory) { + this(connectionFactory, /*maxConnections=*/ 0); + } + + public ReferenceCountedChannel(ChannelConnectionFactory connectionFactory, int maxConnections) { this.dynamicConnectionPool = - new DynamicConnectionPool(connectionFactory, connectionFactory.maxConcurrency()); + new DynamicConnectionPool(connectionFactory, connectionFactory.maxConcurrency(), maxConnections); } public boolean isShutdown() { @@ -87,12 +91,12 @@ public void start(Listener responseListener, Metadata headers) { responseListener) { @Override public void onClose(Status status, Metadata trailers) { - super.onClose(status, trailers); - try { connection.close(); } catch (IOException e) { throw new AssertionError(e.getMessage(), e); + } finally { + super.onClose(status, trailers); } } }, diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index 6ad97fb2a1ece2..39208ead51be11 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -367,7 +367,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { remoteOptions.remoteProxy, authAndTlsOptions, interceptors.build(), - maxConcurrencyPerConnection)); + maxConcurrencyPerConnection), + remoteOptions.remoteMaxConnections); // Create a separate channel if --remote_executor and --remote_cache point to different // endpoints. @@ -390,7 +391,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { remoteOptions.remoteProxy, authAndTlsOptions, interceptors.build(), - maxConcurrencyPerConnection)); + maxConcurrencyPerConnection), + remoteOptions.remoteMaxConnections); } if (enableRemoteDownloader) { @@ -411,7 +413,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { remoteOptions.remoteProxy, authAndTlsOptions, interceptors.build(), - maxConcurrencyPerConnection)); + maxConcurrencyPerConnection), + remoteOptions.remoteMaxConnections); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/grpc/DynamicConnectionPool.java b/src/main/java/com/google/devtools/build/lib/remote/grpc/DynamicConnectionPool.java index 6824563d737a38..39b17c6c2e30a9 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/grpc/DynamicConnectionPool.java +++ b/src/main/java/com/google/devtools/build/lib/remote/grpc/DynamicConnectionPool.java @@ -30,6 +30,7 @@ public class DynamicConnectionPool implements ConnectionPool { private final ConnectionFactory connectionFactory; private final int maxConcurrencyPerConnection; + private final int maxConnections; private final AtomicBoolean closed = new AtomicBoolean(false); @GuardedBy("this") @@ -40,8 +41,14 @@ public class DynamicConnectionPool implements ConnectionPool { public DynamicConnectionPool( ConnectionFactory connectionFactory, int maxConcurrencyPerConnection) { + this(connectionFactory, maxConcurrencyPerConnection, /*maxConnections=*/ 0); + } + + public DynamicConnectionPool( + ConnectionFactory connectionFactory, int maxConcurrencyPerConnection, int maxConnections) { this.connectionFactory = connectionFactory; this.maxConcurrencyPerConnection = maxConcurrencyPerConnection; + this.maxConnections = maxConnections; this.factories = new ArrayList<>(); } @@ -61,12 +68,19 @@ public void close() throws IOException { } } + @GuardedBy("this") + private SharedConnectionFactory nextFactory() { + int index = Math.abs(indexTicker % factories.size()); + indexTicker += 1; + return factories.get(index); + } + /** - * Performs a simple round robin on the list of {@link SharedConnectionFactory} and return one - * having available connections at this moment. + * Performs a simple round robin on the list of {@link SharedConnectionFactory}. * - *

If no factory has available connections, it will create a new {@link - * SharedConnectionFactory}. + *

This will try to find a factory that has available connections at this moment. If no factory + * has available connections, and the number of factories is less than {@link #maxConnections}, it + * will create a new {@link SharedConnectionFactory}. */ private SharedConnectionFactory nextAvailableFactory() { if (closed.get()) { @@ -75,19 +89,20 @@ private SharedConnectionFactory nextAvailableFactory() { synchronized (this) { for (int times = 0; times < factories.size(); ++times) { - int index = Math.abs(indexTicker % factories.size()); - indexTicker += 1; - - SharedConnectionFactory factory = factories.get(index); + SharedConnectionFactory factory = nextFactory(); if (factory.numAvailableConnections() > 0) { return factory; } } - SharedConnectionFactory factory = - new SharedConnectionFactory(connectionFactory, maxConcurrencyPerConnection); - factories.add(factory); - return factory; + if (maxConnections <= 0 || factories.size() < maxConnections) { + SharedConnectionFactory factory = + new SharedConnectionFactory(connectionFactory, maxConcurrencyPerConnection); + factories.add(factory); + return factory; + } else { + return nextFactory(); + } } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index 89d6e1c6f5120f..1bb45daadc13fc 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -59,13 +59,13 @@ public final class RemoteOptions extends OptionsBase { @Option( name = "remote_max_connections", - defaultValue = "100", + defaultValue = "0", documentationCategory = OptionDocumentationCategory.REMOTE, effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS}, help = - "The max. number of concurrent network connections to the remote cache/executor. By " - + "default Bazel limits the number of TCP connections to 100. Setting this flag to " - + "0 will make Bazel choose the number of connections automatically.") + "The max. number of concurrent network connections to the remote cache/executor. By" + + " default the value is set to 0 which will make Bazel choose the number of" + + " connections automatically.") public int remoteMaxConnections; @Option( From f385d1f24193471cdaf5ca21029919eaa280c6a4 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Wed, 3 Nov 2021 12:29:21 +0800 Subject: [PATCH 2/8] Add --incompatible_remote_max_connections_grpc --- .../build/lib/remote/RemoteModule.java | 17 ++++++++++++++--- .../lib/remote/options/RemoteOptions.java | 19 +++++++++++++++---- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index 39208ead51be11..cb3d7976909a86 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -352,6 +352,17 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { // based on the resolved IPs of that server. We assume servers normally have 2 IPs. So the // max concurrency per connection is 100. int maxConcurrencyPerConnection = 100; + int maxConnections = 0; + if (remoteOptions.incompatibleRemoteMaxConnectionsGrpc + && remoteOptions.remoteMaxConnections > 0) { + maxConnections = + (int) + Math.ceil( + (double) remoteOptions.remoteMaxConnections + / (double) maxConcurrencyPerConnection); + maxConcurrencyPerConnection = + (int) Math.ceil((double) remoteOptions.remoteMaxConnections / (double) maxConnections); + } if (enableRemoteExecution) { ImmutableList.Builder interceptors = ImmutableList.builder(); @@ -368,7 +379,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { authAndTlsOptions, interceptors.build(), maxConcurrencyPerConnection), - remoteOptions.remoteMaxConnections); + maxConnections); // Create a separate channel if --remote_executor and --remote_cache point to different // endpoints. @@ -392,7 +403,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { authAndTlsOptions, interceptors.build(), maxConcurrencyPerConnection), - remoteOptions.remoteMaxConnections); + maxConnections); } if (enableRemoteDownloader) { @@ -414,7 +425,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { authAndTlsOptions, interceptors.build(), maxConcurrencyPerConnection), - remoteOptions.remoteMaxConnections); + maxConnections); } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index 1bb45daadc13fc..f6f8f676a99212 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -59,15 +59,26 @@ public final class RemoteOptions extends OptionsBase { @Option( name = "remote_max_connections", - defaultValue = "0", + defaultValue = "100", documentationCategory = OptionDocumentationCategory.REMOTE, effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS}, help = - "The max. number of concurrent network connections to the remote cache/executor. By" - + " default the value is set to 0 which will make Bazel choose the number of" - + " connections automatically.") + "The max. number of concurrent network requests to the HTTP remote cache. By default the" + + " value is 100. Setting this to 0 will make Bazel choose the number of connections" + + " automatically.\n" + + "If --incompatible_remote_max_connections_grpc is set, this also applies" + + " to the gRPC remote cache/executor.") public int remoteMaxConnections; + @Option( + name = "incompatible_remote_max_connections_grpc", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.UNKNOWN}, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + help = "If set to true, --remote_max_connections also applies to gRPC cache/executor.") + public boolean incompatibleRemoteMaxConnectionsGrpc; + @Option( name = "remote_executor", defaultValue = "null", From 35ae5681a7efacb5460b4922ee6476ba51fdc6c5 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Wed, 3 Nov 2021 13:16:50 +0800 Subject: [PATCH 3/8] Update help text. --- .../build/lib/remote/options/RemoteOptions.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index f6f8f676a99212..da1ac3e03e13d6 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -63,11 +63,16 @@ public final class RemoteOptions extends OptionsBase { documentationCategory = OptionDocumentationCategory.REMOTE, effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS}, help = - "The max. number of concurrent network requests to the HTTP remote cache. By default the" + "Limit the max number of concurrent requests to remote cache/executor. By default the" + " value is 100. Setting this to 0 will make Bazel choose the number of connections" + " automatically.\n" - + "If --incompatible_remote_max_connections_grpc is set, this also applies" - + " to the gRPC remote cache/executor.") + + "For HTTP remote cache, one TCP connection could handle one request at one time, so" + + " it could open up to --remote_max_connections TCP connections.\n" + + "If --incompatible_remote_max_connections_grpc is set, this also applies to the" + + " gRPC remote cache/executor, for which, one TCP connection could usually handle" + + " 100+ concurrent requests, so it could open up to `--remote_max_connections / 100`" + + " gRPC channels. Each gRPC channel could open 1 or more TCP connections depending" + + " on the number of resolved server IPs.") public int remoteMaxConnections; @Option( From 9416368c4ccf4bd1b3be8c8d1fca95cb342f33fe Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Wed, 3 Nov 2021 13:19:32 +0800 Subject: [PATCH 4/8] Update help text. --- .../devtools/build/lib/remote/options/RemoteOptions.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index da1ac3e03e13d6..06c01ea735ec06 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -64,8 +64,7 @@ public final class RemoteOptions extends OptionsBase { effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS}, help = "Limit the max number of concurrent requests to remote cache/executor. By default the" - + " value is 100. Setting this to 0 will make Bazel choose the number of connections" - + " automatically.\n" + + " value is 100. Setting this to 0 means no limitation.\n" + "For HTTP remote cache, one TCP connection could handle one request at one time, so" + " it could open up to --remote_max_connections TCP connections.\n" + "If --incompatible_remote_max_connections_grpc is set, this also applies to the" From 6f8de4edc592f59814a8e5593129162f911bea84 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Wed, 3 Nov 2021 13:21:23 +0800 Subject: [PATCH 5/8] Update help text. --- .../google/devtools/build/lib/remote/options/RemoteOptions.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index 06c01ea735ec06..6fbee69b0ca256 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -68,7 +68,7 @@ public final class RemoteOptions extends OptionsBase { + "For HTTP remote cache, one TCP connection could handle one request at one time, so" + " it could open up to --remote_max_connections TCP connections.\n" + "If --incompatible_remote_max_connections_grpc is set, this also applies to the" - + " gRPC remote cache/executor, for which, one TCP connection could usually handle" + + " gRPC remote cache/executor, for which, one gRPC channel could usually handle" + " 100+ concurrent requests, so it could open up to `--remote_max_connections / 100`" + " gRPC channels. Each gRPC channel could open 1 or more TCP connections depending" + " on the number of resolved server IPs.") From 41313388333115edd0b908f25e67cee3213be87f Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Wed, 3 Nov 2021 13:24:18 +0800 Subject: [PATCH 6/8] Update help text. --- .../devtools/build/lib/remote/options/RemoteOptions.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index 6fbee69b0ca256..9e1b585fc3de5c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -68,10 +68,10 @@ public final class RemoteOptions extends OptionsBase { + "For HTTP remote cache, one TCP connection could handle one request at one time, so" + " it could open up to --remote_max_connections TCP connections.\n" + "If --incompatible_remote_max_connections_grpc is set, this also applies to the" - + " gRPC remote cache/executor, for which, one gRPC channel could usually handle" - + " 100+ concurrent requests, so it could open up to `--remote_max_connections / 100`" - + " gRPC channels. Each gRPC channel could open 1 or more TCP connections depending" - + " on the number of resolved server IPs.") + + " gRPC remote cache/executor. One gRPC channel could usually handle 100+ concurrent" + + " requests. We assume the number is 100, so it could open up to" + + " `--remote_max_connections / 100` gRPC channels. Each gRPC channel could open 1 or" + + " more TCP connections depending on the number of resolved server IPs.") public int remoteMaxConnections; @Option( From 43cd7fd0b6ddea44ebe4cd16a8eb895ee3e0b6b6 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Wed, 3 Nov 2021 13:26:16 +0800 Subject: [PATCH 7/8] Update help text. --- .../devtools/build/lib/remote/options/RemoteOptions.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index 9e1b585fc3de5c..c1644e9fd383a4 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -67,8 +67,8 @@ public final class RemoteOptions extends OptionsBase { + " value is 100. Setting this to 0 means no limitation.\n" + "For HTTP remote cache, one TCP connection could handle one request at one time, so" + " it could open up to --remote_max_connections TCP connections.\n" - + "If --incompatible_remote_max_connections_grpc is set, this also applies to the" - + " gRPC remote cache/executor. One gRPC channel could usually handle 100+ concurrent" + + "If --incompatible_remote_max_connections_grpc is set, this also applies to gRPC" + + " remote cache/executor. One gRPC channel could usually handle 100+ concurrent" + " requests. We assume the number is 100, so it could open up to" + " `--remote_max_connections / 100` gRPC channels. Each gRPC channel could open 1 or" + " more TCP connections depending on the number of resolved server IPs.") From 367f20e942f04a67e54e931991cf79973216fc32 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Fri, 5 Nov 2021 13:13:35 +0800 Subject: [PATCH 8/8] Limit concurrent connections instead of requests --- .../build/lib/remote/RemoteModule.java | 11 ++-------- .../lib/remote/options/RemoteOptions.java | 21 +++++-------------- 2 files changed, 7 insertions(+), 25 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index cb3d7976909a86..5b857ca0eaced4 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -353,15 +353,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException { // max concurrency per connection is 100. int maxConcurrencyPerConnection = 100; int maxConnections = 0; - if (remoteOptions.incompatibleRemoteMaxConnectionsGrpc - && remoteOptions.remoteMaxConnections > 0) { - maxConnections = - (int) - Math.ceil( - (double) remoteOptions.remoteMaxConnections - / (double) maxConcurrencyPerConnection); - maxConcurrencyPerConnection = - (int) Math.ceil((double) remoteOptions.remoteMaxConnections / (double) maxConnections); + if (remoteOptions.remoteMaxConnections > 0) { + maxConnections = remoteOptions.remoteMaxConnections; } if (enableRemoteExecution) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index c1644e9fd383a4..657b81fdd9e5b1 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -63,26 +63,15 @@ public final class RemoteOptions extends OptionsBase { documentationCategory = OptionDocumentationCategory.REMOTE, effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS}, help = - "Limit the max number of concurrent requests to remote cache/executor. By default the" + "Limit the max number of concurrent connections to remote cache/executor. By default the" + " value is 100. Setting this to 0 means no limitation.\n" + "For HTTP remote cache, one TCP connection could handle one request at one time, so" - + " it could open up to --remote_max_connections TCP connections.\n" - + "If --incompatible_remote_max_connections_grpc is set, this also applies to gRPC" - + " remote cache/executor. One gRPC channel could usually handle 100+ concurrent" - + " requests. We assume the number is 100, so it could open up to" - + " `--remote_max_connections / 100` gRPC channels. Each gRPC channel could open 1 or" - + " more TCP connections depending on the number of resolved server IPs.") + + " Bazel could make up to --remote_max_connections concurrent requests.\n" + + "For gRPC remote cache/executor, one gRPC channel could usually handle 100+" + + " concurrent requests, so Bazel could make around `--remote_max_connections * 100`" + + " concurrent requests.") public int remoteMaxConnections; - @Option( - name = "incompatible_remote_max_connections_grpc", - defaultValue = "false", - documentationCategory = OptionDocumentationCategory.REMOTE, - effectTags = {OptionEffectTag.UNKNOWN}, - metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, - help = "If set to true, --remote_max_connections also applies to gRPC cache/executor.") - public boolean incompatibleRemoteMaxConnectionsGrpc; - @Option( name = "remote_executor", defaultValue = "null",