[scheduler] 3/n Use a linked list instead of map and queue for callback storage #12893

flarnie · 2018-05-23T20:40:11Z

Edit: Rebased and now there is no cruft on this PR. :)

what is the change?:
See title

why make this change?:
This seems to make the code simpler, and potentially saves space of
having an array and object around holding references to the callbacks.

test plan:
Run existing the new and improved tests, from the previous PR.

acdlite

Left some comments, none of them are too serious but it'd be cool to discuss them before merging.

acdlite · 2018-05-24T17:27:56Z

packages/react-scheduler/src/ReactScheduler.js

 |};

+export type CallbackIdType = any;
+


Why is this any instead of CallbackConfigType?

Let's discuss this in person - the tldr is that we sometimes want the 'callbackIdType' to be a number, sometimes the CallbackConfigType, sometimes something else.

acdlite · 2018-05-24T17:38:13Z

packages/react-scheduler/src/ReactScheduler.js

+        headOfPendingCallbacksLinkedList.previousCallbackConfig = null;
+      }
+      if (tailOfPendingCallbacksLinkedList === latestCallbackConfig) {
+        tailOfPendingCallbacksLinkedList = null;


This the same as the else block of the previous if statement. If there's no next callback in the list, then the list must be empty now.

That's true - if it's more clear will write it that way.

acdlite · 2018-05-24T17:38:45Z

packages/react-scheduler/src/ReactScheduler.js

+      // move head of list to next callback
+      headOfPendingCallbacksLinkedList =
+        latestCallbackConfig.nextCallbackConfig;
+      if (headOfPendingCallbacksLinkedList) {


Make sure we always explicitly compare to null

acdlite · 2018-05-24T17:42:09Z

packages/react-scheduler/src/ReactScheduler.js

+     */
+    const previousCallbackConfig = callbackConfig.previousCallbackConfig;
+    const nextCallbackConfig = callbackConfig.nextCallbackConfig;
+    if (previousCallbackConfig) {


Same here, compare against null

I find this much easier to follow using nested if statements. The reason I prefer that style is it makes it easier to avoid redundant type checks.

if (callback === head) { if (callback === tail) { // List is now empty head = tail = null; } else { // Remove from start of list head = callback.next; // (Flow will complain that `head` could be null) head.previous = null; } } else if (callback === tail) { // Remove from end of list tail = callback.previous; // (Flow will complain that `tail` could be null) tail.next = null; } else { // Remove from middle of list const previous = callback.previous; const next = callback.next; // (Flow will complain that `previous` and `next` could be null) previous.next = next; next.previous = previous; }

But it seems like maybe the redundant checks are necessary to satisfy Flow? That's a shame.

I can try to rewrite it this way, think we can still make Flow happy.
Might be ok with concise > clear in this particular module, but for now let me try the more clear way.

flarnie · 2018-05-25T19:58:22Z

Looks like we can't just remove the ExecutionEnvironment.canUseDOM condition - the ReactDOM-test.js threw because window was undefined.

I'll investigate and see if we can just polyfill the schedule module itself in that test.

flarnie · 2018-05-25T20:21:43Z

Ah - we need to support importing this module even in a node environment, that's what the test is for after all. Glad that test is still there.
I'm going to split the fallback into a separate module and import it at the React level since this is just about supporting isomorphic components, don't think all callers need this to be included in the implementation.

flarnie · 2018-05-25T20:35:50Z

Whatever I do to solve this problem with supporting the node environment, it brings up the question of where this scheduler is intended to work. - we need to confirm whether we will make this scheduler target only web or if it's intended to be cross platform. If so then we should move towards dependency injection and we won't have to worry about whether web APIs are available.

pull-bot · 2018-05-25T21:29:50Z

ReactDOM: size: 🔺+0.2%, gzip: 🔺+0.1%

Details of bundled changes.

Comparing: e7bd3d5...4785e74

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	+0.3%	+0.2%	622.03 KB	623.98 KB	144.87 KB	145.15 KB	UMD_DEV
react-dom.production.min.js	🔺+0.2%	🔺+0.1%	93.71 KB	93.88 KB	30.34 KB	30.37 KB	UMD_PROD
react-dom.development.js	+0.3%	+0.2%	606.4 KB	608.34 KB	140.86 KB	141.15 KB	NODE_DEV
react-dom.production.min.js	🔺+0.2%	🔺+0.1%	92.21 KB	92.38 KB	29.31 KB	29.35 KB	NODE_PROD
ReactDOM-dev.js	+0.3%	+0.2%	628.68 KB	630.62 KB	143.62 KB	143.89 KB	FB_WWW_DEV
ReactDOM-prod.js	🔺+0.3%	🔺+0.2%	273.59 KB	274.44 KB	51.52 KB	51.64 KB	FB_WWW_PROD

react-art

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-art.development.js	+0.5%	+0.4%	404.31 KB	406.25 KB	90.31 KB	90.63 KB	UMD_DEV
react-art.production.min.js	🔺+0.2%	🔺+0.1%	80.71 KB	80.88 KB	24.88 KB	24.92 KB	UMD_PROD
react-art.development.js	+0.6%	+0.4%	330.16 KB	332.11 KB	71.41 KB	71.69 KB	NODE_DEV
react-art.production.min.js	🔺+0.4%	🔺+0.1%	45.07 KB	45.24 KB	14.01 KB	14.03 KB	NODE_PROD
ReactART-dev.js	+0.6%	+0.4%	335.68 KB	337.61 KB	70.79 KB	71.07 KB	FB_WWW_DEV
ReactART-prod.js	🔺+0.6%	🔺+0.4%	146.7 KB	147.55 KB	25.32 KB	25.42 KB	FB_WWW_PROD

react-scheduler

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-scheduler.development.js	+13.8%	+5.7%	13.76 KB	15.67 KB	4.57 KB	4.83 KB	UMD_DEV
react-scheduler.production.min.js	🔺+8.5%	🔺+3.0%	2.09 KB	2.27 KB	1.04 KB	1.07 KB	UMD_PROD
react-scheduler.development.js	+14.0%	+5.8%	13.57 KB	15.47 KB	4.52 KB	4.79 KB	NODE_DEV
react-scheduler.production.min.js	🔺+8.4%	🔺+2.2%	2.18 KB	2.36 KB	1.07 KB	1.09 KB	NODE_PROD

Generated by 🚫 dangerJS

acdlite · 2018-05-25T22:55:13Z

Hmm the bundle size went up more than I expected

acdlite

Other than some code size nits, looks good to go!

Property names are harder to minify; I don't think our build script mangles them. So we should minimize the number of locations where we access properties by assigning them to variables.

For example, instead of:

if (obj.longPropertyName !== null) {
  doSomething(obj.longPropertyName);
}

Do this instead:

const l = obj.longPropertyName;
if (l !== null) {
  doSomething(l);
}

Could you also consider shorter names for the callback config properties?

acdlite · 2018-05-25T23:08:13Z

packages/react-scheduler/src/ReactScheduler.js

+     */
+    if (callbackConfig.nextCallbackConfig !== null) {
+      // we have a nextCallbackConfig
+      const nextCallbackConfig = callbackConfig.nextCallbackConfig;


Code size nits incoming:

Lift this line above the condition so you only have to read callbackConfig.nextCallbackConfig once.

acdlite · 2018-05-25T23:08:25Z

packages/react-scheduler/src/ReactScheduler.js

+
+      if (callbackConfig.previousCallbackConfig !== null) {
+        // we have a previousCallbackConfig
+        const previousCallbackConfig = callbackConfig.previousCallbackConfig;


Same here. Lift this above the conditional.

acdlite · 2018-05-25T23:09:11Z

packages/react-scheduler/src/ReactScheduler.js

+
+      if (callbackConfig.previousCallbackConfig !== null) {
+        // we have a previousCallbackConfig
+        const previousCallbackConfig = callbackConfig.previousCallbackConfig;


And here.

In fact, since we always end up checking these values, we can move the variables to the top of the function.

flarnie · 2018-05-26T21:22:39Z

Sweet! Will take your recommendations about how to slim down this module.

NOTE: This PR depends on facebook#12880 and facebook#12884 Please review those first, and after they land Flarnie will rebase on top of them. --- **what is the change?:** See title **why make this change?:** This seems to make the code simpler, and potentially saves space of having an array and object around holding references to the callbacks. **test plan:** Run existing tests

**what is the change?:** - Removed conditional which fell back to 'setTimeout' when the environment doesn't have DOM. This appears to be an old polyfill used for test environments and we don't use it any more. - Fixed type definitions around the callbackID to be more accurate in the scheduler itself, and more loose in the React code. **why make this change?:** To get Flow passing, simplify the scheduler code, make things accurate. **test plan:** Run tests and flow.

**what is the change?:** Adding verification that 'previousCallbackConfig' and 'nextCallbackConfig' are not null before accessing properties on them. Slightly concerned because this implementation relies on these properties being untouched and correct on the config which is passed to 'cancelScheduledWork' but I guess we already rely heavily on that for this whole approach. :\ **why make this change?:** To get Flow passing. Not sure why it passed earlier and in CI, but now it's not. **test plan:** `yarn flow dom` and other flow tests, lint, tests, etc.

**what is the change?:** We had tried removing the fallback implementation of `scheduler` but tests reminded us that this is important for supporting isomorphic uses of React. Long term we will move this out of the `schedule` module but for now let's keep things simple. **why make this change?:** Keep things working! **test plan:** Ran tests and flow

**what is the change?:** `previousScheduledCallback` -> `prev` `nextScheduledCallback` -> `next` **why make this change?:** We want this package to be smaller, and less letters means less code means smaller! **test plan:** ran existing tests

…ck storage (#12893) * [schedule] Use linked list instead of queue and map for storing cbs NOTE: This PR depends on facebook/react#12880 and facebook/react#12884 Please review those first, and after they land Flarnie will rebase on top of them. --- **what is the change?:** See title **why make this change?:** This seems to make the code simpler, and potentially saves space of having an array and object around holding references to the callbacks. **test plan:** Run existing tests * minor style improvements * refactor conditionals in cancelScheduledWork for increased clarity * Remove 'canUseDOM' condition and fix some flow issues w/callbackID type **what is the change?:** - Removed conditional which fell back to 'setTimeout' when the environment doesn't have DOM. This appears to be an old polyfill used for test environments and we don't use it any more. - Fixed type definitions around the callbackID to be more accurate in the scheduler itself, and more loose in the React code. **why make this change?:** To get Flow passing, simplify the scheduler code, make things accurate. **test plan:** Run tests and flow. * Rewrite 'cancelScheduledWork' so that Flow accepts it **what is the change?:** Adding verification that 'previousCallbackConfig' and 'nextCallbackConfig' are not null before accessing properties on them. Slightly concerned because this implementation relies on these properties being untouched and correct on the config which is passed to 'cancelScheduledWork' but I guess we already rely heavily on that for this whole approach. :\ **why make this change?:** To get Flow passing. Not sure why it passed earlier and in CI, but now it's not. **test plan:** `yarn flow dom` and other flow tests, lint, tests, etc. * ran prettier * Put back the fallback implementation of scheduler for node environment **what is the change?:** We had tried removing the fallback implementation of `scheduler` but tests reminded us that this is important for supporting isomorphic uses of React. Long term we will move this out of the `schedule` module but for now let's keep things simple. **why make this change?:** Keep things working! **test plan:** Ran tests and flow * Shorten properties stored in objects by sheduler **what is the change?:** `previousScheduledCallback` -> `prev` `nextScheduledCallback` -> `next` **why make this change?:** We want this package to be smaller, and less letters means less code means smaller! **test plan:** ran existing tests * further remove extra lines in scheduler

flarnie requested review from acdlite and sebmarkbage May 23, 2018 20:40

facebook-github-bot added the CLA Signed label May 23, 2018

flarnie mentioned this pull request May 24, 2018

[scheduler] 4/n Allow splitting out schedule in fb-www, prepare to fix polyfill issue internally #12900

Merged

acdlite reviewed May 24, 2018

View reviewed changes

flarnie force-pushed the useYouALinkedListForGreatGood4 branch from ee89eb2 to e90359a Compare May 24, 2018 21:15

acdlite approved these changes May 25, 2018

View reviewed changes

flarnie added 8 commits May 26, 2018 14:23

minor style improvements

eaf2764

refactor conditionals in cancelScheduledWork for increased clarity

d883e84

ran prettier

f69b6c2

flarnie force-pushed the useYouALinkedListForGreatGood4 branch from 75377a9 to e123aec Compare May 26, 2018 21:37

further remove extra lines in scheduler

4785e74

flarnie merged commit 61777a7 into facebook:master May 26, 2018

[scheduler] 3/n Use a linked list instead of map and queue for callback storage #12893

[scheduler] 3/n Use a linked list instead of map and queue for callback storage #12893

Uh oh!

Conversation

flarnie commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acdlite left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flarnie commented May 25, 2018

Uh oh!

flarnie commented May 25, 2018

Uh oh!

flarnie commented May 25, 2018

Uh oh!

pull-bot commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

react-dom

react-art

react-scheduler

Uh oh!

acdlite commented May 25, 2018

Uh oh!

acdlite left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flarnie commented May 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

flarnie commented May 23, 2018 •

edited

Loading

pull-bot commented May 25, 2018 •

edited

Loading

acdlite left a comment •

edited

Loading