feat: allow specifying how many idle GRPC channels to keep #837

schmidt-sebastian · 2019-12-26T11:44:42Z

This PR lets users influence the memory profile of their Firestore usage:

They can set the minimum client pool size to 0 to close all GRPC channels when Firestore is completely idle
They can use a larger number to reduce the startup costs of new GRPC channels.

This PR aims to preserve the current behavior by always leaving at least one GRPC channel if no option is provided.

b/146363799

codecov · 2019-12-26T11:53:42Z

Codecov Report

Merging #837 into master will decrease coverage by <.01%.
The diff coverage is 89.28%.

@@            Coverage Diff             @@
##           master     #837      +/-   ##
==========================================
- Coverage   88.83%   88.82%   -0.01%     
==========================================
  Files          27       27              
  Lines       16725    16765      +40     
  Branches     1151     1156       +5     
==========================================
+ Hits        14858    14892      +34     
- Misses       1862     1868       +6     
  Partials        5        5

Impacted Files	Coverage Δ
types/firestore.d.ts	`0% <0%> (ø)`	⬆️
dev/src/index.ts	`98.74% <100%> (+0.01%)`	⬆️
dev/src/types.ts	`100% <100%> (ø)`	⬆️
dev/src/pool.ts	`96.61% <97.87%> (+1.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c870e6...0e34859. Read the comment docs.

mikelehen

Mostly nits...

As an aside, I will just point out that it seems unfortunate that we have to expose this to developers and make them make a trade-off between latency, memory, and the ability for the client to 100% shut down (if I'm understanding correctly). I worry that the only way people will find out about this options is when they run into some problem, diagnose it, and then go googling for it and find a post telling them to change this. That means many people will just suffer through the problem (because googling / solving problems is hard) and those that do solve it will have had to go on an unwanted adventure to diagnose / solve the problem.

All that is to say: We should do our best to make this "just work" for as many use cases as possible so 99.9% of people don't need to go on this adventure. :-)

mikelehen · 2019-12-26T16:47:10Z

dev/src/pool.ts

-    this.activeClients.forEach((requestCount, client) => {
-      if (!selectedClient && requestCount < this.concurrentOperationLimit) {
+    for (const [client, requestCount] of this.activeClients) {
+      // Bin pack requests to reduce the maximize the number of idle clients as


"reduce the maximize the ..."? Also, I don't do bin packing enough for this comment to 100% make sense right away. And I had to think more than 3 seconds to figure out what "selectedRequestCount" was.

Perhaps rename selectedRequestCount to selectedClientRequestCount (long/verbose, I know, sorry!) and the comment to something like:

Use the "most-full" client that can still accommodate the required requestCount in order to maximize the number of idle clients as operations start to complete.

Don't feel very strongly though.

Your comment is not only grammatically correct, but also more helpful. Thanks!

mikelehen · 2019-12-26T16:47:38Z

dev/src/pool.ts

@@ -77,7 +85,7 @@ export class ClientPool<T> {
        selectedClient = client;


The log line above is going to get spewed multiple times now. I think it should be extracted from the for-loop.

Good catch. I moved it outside the loop.

mikelehen · 2019-12-26T16:50:45Z

dev/src/pool.ts

    assert(requestCount > 0, 'No active request');
+    this.activeClients.set(client, requestCount! - 1);


Seems odd you need the ! but I assume you have your reasons...

Uhm. I hope I had my reasons when I added it first, but they are not immediately apparent now. Removed.

mikelehen · 2019-12-26T17:02:01Z

dev/src/pool.ts

+    //   least one active client as a single client can never exceed the
+    //   concurrent operation limit by itself).
+    // - Use `maxIdleClients:0` to shut down the client pool completely when all
+    //   clients are idle.


FWIW I read this comment 2 or 3 times, and read the code once, and the code still makes more sense to me than the comment. So it might be worth rephrasing / paring down the comment. I'm not 100% sure what it's trying to get across. Is it justifying the existence of the maxIdleClients setting (in which maybe the bulk of this comment should go there?)? Or is there actually some nuance to this code that needs 10 lines of explanation?

I was trying to justify that the code doesn't just count the number of idle clients, but instead counts the number of idle "request slots". If this comment doesn't help, then I am ok with removing it.

mikelehen · 2019-12-26T17:19:16Z

dev/src/types.ts

+  /**
+   * The maximum number of idle GRPC channels to keep. A smaller number of idle
+   * channels reduces memory usage but increases request latency for clients
+   * with fluctuating request rates. Defaults to 1.


Elsewhere you document that setting it to 0 allows the client pool to shut down completely. Is that a behavior that should be documented here?

That's a good suggestion. Added (here and in the other two places...)

schmidt-sebastian

Thanks for your review. There was no rush for this :)

The default setting should keep the existing behavior and work for most users. There have been a couple complaints about our memory usage from very heavy users. I don't expect maxIdleChannels to be an often used setting, but it should allow users that want to tune memory usage to do so.

schmidt-sebastian · 2019-12-27T01:26:10Z

dev/src/pool.ts

-    this.activeClients.forEach((requestCount, client) => {
-      if (!selectedClient && requestCount < this.concurrentOperationLimit) {
+    for (const [client, requestCount] of this.activeClients) {
+      // Bin pack requests to reduce the maximize the number of idle clients as


Your comment is not only grammatically correct, but also more helpful. Thanks!

schmidt-sebastian · 2019-12-27T01:26:23Z

dev/src/pool.ts

@@ -77,7 +85,7 @@ export class ClientPool<T> {
        selectedClient = client;


Good catch. I moved it outside the loop.

schmidt-sebastian · 2019-12-27T01:28:05Z

dev/src/pool.ts

    assert(requestCount > 0, 'No active request');
+    this.activeClients.set(client, requestCount! - 1);


Uhm. I hope I had my reasons when I added it first, but they are not immediately apparent now. Removed.

schmidt-sebastian · 2019-12-27T01:29:31Z

dev/src/pool.ts

+    //   least one active client as a single client can never exceed the
+    //   concurrent operation limit by itself).
+    // - Use `maxIdleClients:0` to shut down the client pool completely when all
+    //   clients are idle.


I was trying to justify that the code doesn't just count the number of idle clients, but instead counts the number of idle "request slots". If this comment doesn't help, then I am ok with removing it.

schmidt-sebastian · 2019-12-27T01:32:09Z

dev/src/types.ts

+  /**
+   * The maximum number of idle GRPC channels to keep. A smaller number of idle
+   * channels reduces memory usage but increases request latency for clients
+   * with fluctuating request rates. Defaults to 1.


That's a good suggestion. Added (here and in the other two places...)

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 26, 2019

schmidt-sebastian force-pushed the mrschmidt/idlechannels branch 2 times, most recently from d8ae845 to 8d94521 Compare December 26, 2019 11:53

schmidt-sebastian force-pushed the mrschmidt/idlechannels branch 2 times, most recently from bb88a59 to 470e450 Compare December 26, 2019 12:01

feat: allow specifying how many idle GRPC channels to keep

e3d7882

schmidt-sebastian force-pushed the mrschmidt/idlechannels branch from 470e450 to e3d7882 Compare December 26, 2019 12:06

schmidt-sebastian added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 26, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 26, 2019

schmidt-sebastian added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 26, 2019

schmidt-sebastian requested a review from mikelehen December 26, 2019 12:19

schmidt-sebastian assigned mikelehen Dec 26, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 26, 2019

mikelehen approved these changes Dec 26, 2019

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Dec 26, 2019

schmidt-sebastian added 2 commits December 27, 2019 09:06

Merge branch 'master' into mrschmidt/idlechannels

5e74ffe

Review

c34cf1b

schmidt-sebastian commented Dec 27, 2019

View reviewed changes

schmidt-sebastian added 4 commits December 27, 2019 09:44

Lint fix

eb13b57

Add compile time test

a20e58a

Increase test timeout

69354ad

Remove space

0e34859

schmidt-sebastian merged commit 37e93da into master Dec 27, 2019

schmidt-sebastian deleted the mrschmidt/idlechannels branch January 3, 2020 00:33

yoshi-automation mentioned this pull request Jan 15, 2020

chore: re-order JSON imports #888

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow specifying how many idle GRPC channels to keep #837

feat: allow specifying how many idle GRPC channels to keep #837

schmidt-sebastian commented Dec 26, 2019 •

edited

codecov bot commented Dec 26, 2019 •

edited

mikelehen left a comment

mikelehen Dec 26, 2019

schmidt-sebastian Dec 27, 2019

mikelehen Dec 26, 2019

schmidt-sebastian Dec 27, 2019

mikelehen Dec 26, 2019

schmidt-sebastian Dec 27, 2019

mikelehen Dec 26, 2019

schmidt-sebastian Dec 27, 2019

mikelehen Dec 26, 2019

schmidt-sebastian Dec 27, 2019

schmidt-sebastian left a comment

schmidt-sebastian Dec 27, 2019

schmidt-sebastian Dec 27, 2019

schmidt-sebastian Dec 27, 2019

schmidt-sebastian Dec 27, 2019

schmidt-sebastian Dec 27, 2019

		@@ -77,7 +85,7 @@ export class ClientPool<T> {
		selectedClient = client;

		assert(requestCount > 0, 'No active request');
		this.activeClients.set(client, requestCount! - 1);

feat: allow specifying how many idle GRPC channels to keep #837

feat: allow specifying how many idle GRPC channels to keep #837

Conversation

schmidt-sebastian commented Dec 26, 2019 • edited

codecov bot commented Dec 26, 2019 • edited

Codecov Report

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian commented Dec 26, 2019 •

edited

codecov bot commented Dec 26, 2019 •

edited