Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9493 +/- ##
==========================================
+ Coverage 87.71% 87.72% +0.01%
==========================================
Files 436 436
Lines 30822 30823 +1
==========================================
+ Hits 27036 27040 +4
+ Misses 3785 3783 -2
+ Partials 1 0 -1
🚀 New features to boost your workflow:
|
|
I understand that the part of invoking a callback defined by tests from non-test code is existing art. But it makes me a bit uncomfortable though. I think we can change this piece of code to not have this kind of coupling with the test code. The only thing that the test needs it the actual amount of quota that we being replenished. Based on the tests, this value is completely computable in the test. If the test does that, then it can atomically read the quota value and compare it against the expected value. And if we do this, we can completely also get rid of the |
writeQuota.initstored the bound method valuew.realReplenishin thereplenishfunc field. Taking a method value that captures its receiver allocates a closure, so this cost one heap allocation per stream on both the client and the server, i.e. 2 allocations per unary RPC. The field only existed so that a test could observe replenishment; production code calls it in a single place (loopyWriter.processData).This PR makes
replenisha regular method and replaces the overridable field with an optional, test-only observer,onReplenishForTesting, which is nil in production and is called in addition to (not instead of) the real flow-control update.TestLargeMessageWithDelayRead, the only user of the old override, is updated to use the hook.Benchmarks
benchmainwith-bufconn -networkMode=Local -compression=off -benchtime=60s. Each round runsmasterand this PR at the same time.Unary, 100 concurrent calls, 0-byte payloads
Streaming, 200 concurrent calls, 100-byte payloads
Streaming is unaffected as expected: streams are long-lived, so the per-stream allocation was already amortized away. A heap profile of the unary benchmark confirms that
writeQuota.initnow only allocates its notification channel.RELEASE NOTES: None