Improve (and shorten) query polling implementation. by benjamn · Pull Request #4243 · apollographql/apollo-client

benjamn · 2018-12-17T17:19:41Z

This implementation has the following benefits:

It collapses the QueryScheduler abstraction into the QueryManager (which was always ultimately responsible for managing the lifetime of polling timers), thus simplifying the relationship between the QueryManager and its ObservableQuery objects.
It's about 100 bytes smaller than the previous implementation, after minification and gzip.
It uses setTimeout rather than setInterval, so event loop starvation never leads to a rapid succession of setInterval catch-up calls.
It guarantees at most one timeout will be pending for an arbitrary number of polling queries, rather than a separate timer for every distinct polling interval.
Fewer independent timers means better batching behavior, usually.
Though there may be a delay between the desired polling time for a given query and the actual polling time, the delay is never greater than the minimum polling interval across all queries, which changes dynamically as polling queries are started and stopped.

This implementation has the following benefits: - It collapses the QueryScheduler abstraction into the QueryManager (which was always ultimately responsible for managing the lifetime of polling timers), thus simplifying the relationship between the QueryManager and its ObservableQuery objects. - It's about 100 bytes smaller than the previous implementation, after minification and gzip. - It uses setTimeout rather than setInterval, so event loop starvation never leads to a rapid succession of setInterval catch-up calls. - It guarantees at most one timeout will be pending for an arbitrary number of polling queries, rather than a separate timer for every distinct polling interval. - Fewer independent timers means better batching behavior, usually. - Though there may be a delay between the desired polling time for a given query and the actual polling time, the delay is never greater than the minimum polling interval across all queries, which changes dynamically as polling queries are started and stopped.

benjamn · 2018-12-17T17:43:52Z

packages/apollo-client/tsconfig.test.json

    "src/core/__tests__/fetchPolicies.ts",
    "src/data/__tests__/queries.ts",
    "src/errors/__tests__/ApolloError.ts",
-    "src/scheduler/__tests__/scheduler.ts"


This small change is deceptively significant, because it means the scheduler.ts tests are now required to pass tsc type checking.

Right - at some point we should probably get all exclude files tsc ready, and remove them from the tsconfig.test.json.

benjamn · 2018-12-17T17:45:02Z

packages/apollo-client/src/core/__tests__/scheduler.ts

+    queryManager.startPollingQuery(queryOptions, queryId);
+
+    let count = 0;
+    queryManager.pollingInfoByQueryId.forEach((info: { interval: number }, qid: string) => {


I don't understand why TypeScript has trouble inferring the type of queryManager.pollingInfoByQueryId here (it assumes any).

TS seems to be able to infer the type when I check on my side, after a full npm run clean; npm i (I'm then able to drop the types in the forEach function signature). Something might just be out synch behind the scenes.

benjamn · 2018-12-17T17:46:17Z

packages/apollo-client/src/core/ObservableQuery.ts


-    if (this.isCurrentlyPolling) {
-      this.scheduler.stopPollingQuery(this.queryId);
-      this.isCurrentlyPolling = false;


No need to keep track of isCurrentlyPolling here in ObservableQuery, because the QueryManager tracks that information in pollingInfoByQueryId, and stopping is no longer required before restarting.

benjamn · 2018-12-17T17:48:06Z

packages/apollo-client/src/core/QueryManager.ts


    return new ObservableQuery<T>({
-      scheduler: this.scheduler,
+      queryManager: this,


I hope it's safe to change ObservableQuery constructor parameters like this. From what I can tell, ObservableQuery is used as a type elsewhere, but this is the only place where new instances are created?

I think it should be safe. We are exporting ObservableQuery, but I doubt anyone is using it externally (for anything other than types) since they'd have to provide their own QueryScheduler instance.

benjamn · 2018-12-17T17:49:19Z

packages/apollo-client/src/core/__tests__/scheduler.ts

+import { ObservableQuery } from '../../core/ObservableQuery';
+
+// Used only for unit testing.
+function registerPollingQuery<T>(


This used to be a method of QueryScheduler, even though it was only used for testing. Turning test-only methods into test functions is an easy way to save bundle size.

hwillson

Awesome @benjamn - looks great! My comments here are just comments on your comments, so nothing new to add. LGTM!

hwillson · 2018-12-18T10:48:24Z

packages/apollo-client/src/core/QueryManager.ts


    return new ObservableQuery<T>({
-      scheduler: this.scheduler,
+      queryManager: this,


I think it should be safe. We are exporting ObservableQuery, but I doubt anyone is using it externally (for anything other than types) since they'd have to provide their own QueryScheduler instance.

hwillson · 2018-12-18T11:01:42Z

packages/apollo-client/src/core/__tests__/scheduler.ts

+    queryManager.startPollingQuery(queryOptions, queryId);
+
+    let count = 0;
+    queryManager.pollingInfoByQueryId.forEach((info: { interval: number }, qid: string) => {


TS seems to be able to infer the type when I check on my side, after a full npm run clean; npm i (I'm then able to drop the types in the forEach function signature). Something might just be out synch behind the scenes.

hwillson · 2018-12-18T11:04:36Z

packages/apollo-client/tsconfig.test.json

    "src/core/__tests__/fetchPolicies.ts",
    "src/data/__tests__/queries.ts",
    "src/errors/__tests__/ApolloError.ts",
-    "src/scheduler/__tests__/scheduler.ts"


Right - at some point we should probably get all exclude files tsc ready, and remove them from the tsconfig.test.json.

This implementation has the following benefits: - It collapses the QueryScheduler abstraction into the QueryManager (which was always ultimately responsible for managing the lifetime of polling timers), thus simplifying the relationship between the QueryManager and its ObservableQuery objects. - It's about 100 bytes smaller than the previous implementation, after minification and gzip. - It uses setTimeout rather than setInterval, so event loop starvation never leads to a rapid succession of setInterval catch-up calls. - It guarantees at most one timeout will be pending for an arbitrary number of polling queries, rather than a separate timer for every distinct polling interval. - Fewer independent timers means better batching behavior, usually. - Though there may be a delay between the desired polling time for a given query and the actual polling time, the delay is never greater than the minimum polling interval across all queries, which changes dynamically as polling queries are started and stopped.

danilobuerger · 2019-01-18T20:29:40Z

Breaks MockedProvider in react-apollo as a public api was removed without major semver bump. See apollographql/react-apollo#2738

This reverts commit b4f0c8e. Should fix apollographql/react-apollo#2738, until we can find a better solution.

This reverts commit 9739ff6. Now that we have a uniform interface for terminating ApolloClient instances (#4336), there should be no need for any external code to access the QueryScheduler abstraction, which this commit removes. We should wait to merge and release this change until after apollographql/react-apollo#2741 has been merged and released, so that we don't break older versions of MockedProvider.

…removal Un-revert "Improve (and shorten) query polling implementation. (#4243)"

@voxtex

The last time I touched this code in #4243, I thought it was worthwhile to preserve the behavior of sometimes batching polling query fetches in the same tick of the event loop. However, as @voxtex points out in #4786, almost any batching strategy will lead to unpredictable fetch timing, skipped fetches, poor accounting for fetch duration (which could exceed the polling interval), and so on. On top of that, even the premise that "batching is good" comes into question if it doesn't happen consistently. An implementation which uses independent timers seems much simpler and more predictable.

@voxtex

The last time I touched this code in #4243, I thought it was worthwhile to preserve the behavior of sometimes batching polling query fetches in the same tick of the event loop. However, as @voxtex points out in #4786, almost any batching strategy will lead to unpredictable fetch timing, skipped fetches, poor accounting for fetch duration (which could exceed the polling interval), and so on. On top of that, even the premise that "batching is good" comes into question if it doesn't happen consistently. An implementation which uses independent timers seems much simpler and more predictable.

benjamn self-assigned this Dec 17, 2018

benjamn requested review from hwillson and jbaxleyiii December 17, 2018 17:19

hwillson added the in progress label Dec 17, 2018

benjamn commented Dec 17, 2018

View reviewed changes

hwillson approved these changes Dec 18, 2018

View reviewed changes

benjamn merged commit b483ee8 into wip-reduce-bundle-size Dec 18, 2018

hwillson removed the in progress label Dec 18, 2018

benjamn added a commit that referenced this pull request Jan 18, 2019

Revert "Improve (and shorten) query polling implementation. (#4243)"

9739ff6

This reverts commit b4f0c8e. Should fix apollographql/react-apollo#2738, until we can find a better solution.

benjamn deleted the simplify-query-polling branch January 22, 2019 18:11

benjamn added a commit that referenced this pull request Jan 22, 2019

Merge pull request #4337 from apollographql/un-revert-QueryScheduler-…

e399ad8

…removal Un-revert "Improve (and shorten) query polling implementation. (#4243)"

benjamn mentioned this pull request May 10, 2019

Stop batching polling query fetches. #4800

Merged

github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023

Conversation

benjamn commented Dec 17, 2018

Uh oh!

benjamn Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

hwillson Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjamn Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

hwillson Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

benjamn Dec 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjamn Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

hwillson Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

benjamn Dec 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hwillson left a comment

Choose a reason for hiding this comment

Uh oh!

hwillson Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

hwillson Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

hwillson Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danilobuerger commented Jan 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hwillson Dec 18, 2018 •

edited

Loading

benjamn Dec 17, 2018 •

edited

Loading

benjamn Dec 17, 2018 •

edited

Loading

hwillson Dec 18, 2018 •

edited

Loading