Benchmark: support -c correctly - #3170
Merged
Merged
Conversation
…t its own readme.md documents `+m`/`-m` as "when enabled clients share a connection, otherwise each client has a separate connection". The classic (old-core) leg did not implement the second half: OldCoreBenchmarkBase built a single ConnectionMultiplexer in its constructor and `GetClient(int index)` ignored the index entirely, returning that one IDatabase to every worker. `Multiplexed` was never read on this leg at all -- `+m` parsed and then did nothing. So `-c` was CONCURRENCY (in-flight requests pipelined onto one connection), not connection count, on the leg that ships. The fan-out above it was always correct: BenchmarkBase allocates ClientCount tasks and calls GetClient(i) per worker; only the worker-to-connection mapping collapsed. A multiplexer is one connection per endpoint by design, so "a connection per client" means a multiplexer per client. Now builds `Multiplexed ? 1 : ClientCount` of them and maps worker i to `i % connectionCount`, so `+m` reproduces the previous behaviour exactly and is the only way to get it. NewCoreBenchmark already did this (`new(count: Multiplexed ? 1 : ClientCount)`); BridgeBenchmark shares this base and inherits the fix. VERIFIED against a live server, sampling DURING the run from two independent vantage points -- the OS TCP table by owning pid, and the server's own CLIENT LIST: -c 1 OS 1,1,1,1 CLIENT LIST 1 (+ the cli itself) -c 8 OS 8,8,8,8 CLIENT LIST 8 -c 50 OS 50,50,50,50 CLIENT LIST 50 -c 50 +m OS 1,1,1,1 CLIENT LIST 1 The `+m` cell is the discriminating one: without it, a fix that simply hard-wired N connections would pass just as happily. `--basic` also exercised (exit 0). AND A BANNER THAT CANNOT AGREE WITH A FLAG THAT DID NOTHING. The per-test banner printed ", mux" whenever `+m` was passed -- including on the leg where it had no effect -- so it reported the FLAG, not the behaviour, and could not have caught this. `ConnectionCount` is virtual, defaulting to the intended count, and OldCoreBenchmarkBase overrides it with `_connectionMultiplexers.Length`: the number actually created. It is printed on the `### ... ###` announce line, which survives `-q` so a harness can gate on it, and in the per-test banner.
This file contains hidden or 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
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.
give each client its own connection unless +m specified
readme.md documents
+m/-mas "when enabled clients share a connection, otherwise each client has a separate connection". The classic (old-core) leg did not implement the second half: OldCoreBenchmarkBase built a single ConnectionMultiplexer in its constructor andGetClient(int index)ignored the index entirely, returning that one IDatabase to every worker.Multiplexedwas never read on this leg at all --+mparsed and then did nothing.So
-cwas CONCURRENCY (in-flight requests pipelined onto one connection), not connection count, on the leg that ships. The fan-out above it was always correct: BenchmarkBase allocates ClientCount tasks and calls GetClient(i) per worker; only the worker-to-connection mapping collapsed.A multiplexer is one connection per endpoint by design, so "a connection per client" means a multiplexer per client. Now builds
Multiplexed ? 1 : ClientCountof them and maps worker i toi % connectionCount, so+mreproduces the previous behaviour exactly and is the only way to get it. NewCoreBenchmark already did this (new(count: Multiplexed ? 1 : ClientCount)); BridgeBenchmark shares this base and inherits the fix.VERIFIED against a live server, sampling DURING the run from two independent vantage points -- the OS TCP table by owning pid, and the server's own CLIENT LIST:
-c 1 OS 1,1,1,1 CLIENT LIST 1 (+ the cli itself)
-c 8 OS 8,8,8,8 CLIENT LIST 8
-c 50 OS 50,50,50,50 CLIENT LIST 50
-c 50 +m OS 1,1,1,1 CLIENT LIST 1
The
+mcell is the discriminating one: without it, a fix that simply hard-wired N connections would pass just as happily.--basicalso exercised (exit 0).AND A BANNER THAT CANNOT AGREE WITH A FLAG THAT DID NOTHING. The per-test banner printed ", mux" whenever
+mwas passed -- including on the leg where it had no effect -- so it reported the FLAG, not the behaviour, and could not have caught this.ConnectionCountis virtual, defaulting to the intended count, and OldCoreBenchmarkBase overrides it with_connectionMultiplexers.Length: the number actually created. It is printed on the### ... ###announce line, which survives-qso a harness can gate on it, and in the per-test banner.Checklist