New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement pauseExecution, continueExecution, dumpQueue for Scheduler #14053

Merged
merged 3 commits into from Dec 6, 2018

Conversation

Projects
None yet
6 participants
@mrkev
Copy link
Contributor

mrkev commented Oct 31, 2018

  • pauseExecution: pauses the scheduler from making progress running subsequent callbacks in the queue.
  • continueExecution: continues executing callbacks from the queue.
  • dumpQueue: returns an array of all scheduled callbacks.

Note: at the moment tests don't pass, because packages/scheduler/src/__tests__/SchedulerUMDBundle-test.internal.js ensures the APIs exported across different modules is consistent, but I only added the functions to umd/scheduler.development. Please advise!

cc @n8schloss

@sizebot

This comment has been minimized.

Copy link

sizebot commented Oct 31, 2018

React: size: 🔺+1.2%, gzip: 🔺+0.9%

Details of bundled changes.

Comparing: 98eb5ae...3c6bb9d

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +1.0% +0.7% 97.03 KB 98.03 KB 25.44 KB 25.63 KB UMD_DEV
react.production.min.js 🔺+1.2% 🔺+0.9% 11.41 KB 11.55 KB 4.54 KB 4.58 KB UMD_PROD
react.profiling.min.js +1.0% +0.8% 13.57 KB 13.7 KB 5.07 KB 5.11 KB UMD_PROFILING
react.development.js +0.1% +0.1% 61.29 KB 61.36 KB 16.53 KB 16.55 KB NODE_DEV
react.production.min.js 0.0% 0.0% 6.11 KB 6.11 KB 2.61 KB 2.62 KB NODE_PROD
React-dev.js 0.0% 0.0% 58.94 KB 58.96 KB 15.78 KB 15.78 KB FB_WWW_DEV
React-prod.js 0.0% 0.0% 15.24 KB 15.24 KB 4.08 KB 4.08 KB FB_WWW_PROD
React-profiling.js 0.0% 0.0% 15.24 KB 15.24 KB 4.08 KB 4.08 KB FB_WWW_PROFILING

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js 0.0% 0.0% 723.46 KB 723.78 KB 167.19 KB 167.25 KB UMD_DEV
react-dom.profiling.min.js 0.0% 0.0% 100.78 KB 100.78 KB 32.54 KB 32.55 KB UMD_PROFILING
react-dom.development.js 0.0% 0.0% 718.76 KB 718.83 KB 165.83 KB 165.85 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 97.79 KB 97.79 KB 31.38 KB 31.38 KB NODE_PROD
react-dom.profiling.min.js 0.0% 0.0% 100.88 KB 100.88 KB 31.99 KB 31.99 KB NODE_PROFILING
ReactDOM-dev.js 0.0% 0.0% 740.21 KB 740.23 KB 166.84 KB 166.85 KB FB_WWW_DEV
ReactDOM-prod.js 0.0% -0.0% 308.18 KB 308.18 KB 56.78 KB 56.78 KB FB_WWW_PROD
react-dom-unstable-fire.development.js 0.0% 0.0% 723.75 KB 724.06 KB 167.3 KB 167.36 KB UMD_DEV
react-dom-unstable-fire.profiling.min.js 0.0% 0.0% 100.8 KB 100.8 KB 32.55 KB 32.55 KB UMD_PROFILING
react-dom-unstable-fire.development.js 0.0% 0.0% 719.04 KB 719.11 KB 165.95 KB 165.97 KB NODE_DEV
react-dom-unstable-fire.production.min.js 0.0% 0.0% 97.8 KB 97.8 KB 31.39 KB 31.39 KB NODE_PROD
react-dom-unstable-fire.profiling.min.js 0.0% 0.0% 100.89 KB 100.89 KB 32 KB 32 KB NODE_PROFILING
ReactFire-dev.js 0.0% 0.0% 739.36 KB 739.38 KB 166.78 KB 166.79 KB FB_WWW_DEV
react-dom-test-utils.development.js 0.0% 0.0% 44.59 KB 44.59 KB 12.24 KB 12.24 KB NODE_DEV
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.61 KB 60.61 KB 15.92 KB 15.92 KB UMD_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 0.0% 11.01 KB 11.01 KB 3.81 KB 3.81 KB UMD_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.29 KB 60.29 KB 15.79 KB 15.79 KB NODE_DEV
react-dom-server.browser.development.js +0.1% +0.1% 123.82 KB 123.89 KB 33.07 KB 33.09 KB UMD_DEV
react-dom-server.browser.production.min.js 0.0% 0.0% 16.99 KB 16.99 KB 6.52 KB 6.53 KB UMD_PROD
react-dom-server.browser.development.js +0.1% +0.1% 119.95 KB 120.02 KB 32.14 KB 32.16 KB NODE_DEV
react-dom-server.browser.production.min.js 0.0% 0.0% 16.9 KB 16.9 KB 6.52 KB 6.52 KB NODE_PROD
ReactDOMServer-dev.js 0.0% 0.0% 121.18 KB 121.2 KB 31.79 KB 31.79 KB FB_WWW_DEV
react-dom-server.node.development.js +0.1% +0.1% 122.01 KB 122.08 KB 32.68 KB 32.7 KB NODE_DEV
react-dom-server.node.production.min.js 0.0% 0.0% 17.77 KB 17.77 KB 6.83 KB 6.83 KB NODE_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.2% 1.05 KB 1.05 KB 637 B 636 B NODE_PROD
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 3.7 KB 3.7 KB 1.42 KB 1.42 KB NODE_DEV

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.1% +0.1% 503.87 KB 504.18 KB 111.06 KB 111.12 KB UMD_DEV
react-art.production.min.js 0.0% 0.0% 89.96 KB 89.96 KB 27.66 KB 27.66 KB UMD_PROD
react-art.development.js 0.0% 0.0% 435.6 KB 435.67 KB 93.99 KB 94.02 KB NODE_DEV
ReactART-dev.js 0.0% 0.0% 443.88 KB 443.9 KB 92.97 KB 92.97 KB FB_WWW_DEV

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js +9.1% +11.1% 21.48 KB 23.44 KB 5.77 KB 6.41 KB NODE_DEV
scheduler.production.min.js 🔺+3.5% 🔺+2.4% 4.57 KB 4.73 KB 1.78 KB 1.83 KB NODE_PROD
Scheduler-dev.js +9.4% +9.8% 21.68 KB 23.71 KB 5.81 KB 6.37 KB FB_WWW_DEV
Scheduler-prod.js 🔺+2.1% 🔺+2.0% 12.88 KB 13.16 KB 2.79 KB 2.84 KB FB_WWW_PROD
scheduler-tracing.development.js +0.7% +1.2% 10.24 KB 10.32 KB 2.36 KB 2.38 KB NODE_DEV
scheduler-tracing.profiling.min.js 0.0% +0.1% 3.26 KB 3.26 KB 999 B 1000 B NODE_PROFILING
SchedulerTracing-dev.js +0.2% +0.6% 10.31 KB 10.33 KB 2.26 KB 2.27 KB FB_WWW_DEV

Generated by 🚫 dangerJS

@mrkev mrkev force-pushed the mrkev:scheduler-debug-funcs branch 3 times, most recently from 6a5766c to e4d6a61 Oct 31, 2018

@@ -31,6 +31,9 @@ var IDLE_PRIORITY = maxSigned31BitInt;
var firstCallbackNode = null;

var currentDidTimeout = false;
// Pausing the scheduler is useful for debugging.
var isSchedulerPaused = false;

This comment has been minimized.

@mrkev

mrkev Nov 5, 2018

Contributor

@acdlite I think I might want to expose this variable too, since for debugging it's necessary to know if the thing is paused at all in the first place

updateTestResult(8, `Queue size: ${dumpQueue().length}.`);
displayTestResult(8);
});
}

This comment has been minimized.

@bvaughn

bvaughn Nov 6, 2018

Contributor

Thanks for adding a new integration test!

this,
arguments
);
}

This comment has been minimized.

@bvaughn

bvaughn Nov 6, 2018

Contributor

I strongly dislike the fact that the newly proposed API methods only exist on the DEV build.

Do other packages do this? I know that sometimes methods are no-ops in production builds, but having them be missing entirely feels weird.

I also think the fact that these methods exist only for DEV reinforces the notion that you could just use existing browser tooling (like the browser's built-in pause/resume functionality).

This comment has been minimized.

@mrkev

mrkev Nov 6, 2018

Contributor

I don't have an opinion on this. no-oping in production seems a good solution in my book.

"size": 95936,
"gzip": 25258
"size": 99473,
"gzip": 25976

This comment has been minimized.

@bvaughn

bvaughn Nov 6, 2018

Contributor

Let's revert the changes to this file.

@@ -336,6 +347,31 @@ function unstable_scheduleCallback(callback, deprecated_options) {
return newNode;
}

function unstable_pauseExecution() {
isSchedulerPaused = true;

This comment has been minimized.

@bvaughn

bvaughn Nov 6, 2018

Contributor

Why isn't the browser's built-in pause/resume functionality sufficient for whatever you're using this for?

}

return callbacks;
}

This comment has been minimized.

@bvaughn

bvaughn Nov 6, 2018

Contributor

Instead of exposing play, pause, and dump-queue– what if we just exposed the queue itself (firstCallbackNode) somewhere globally in DEV mode only? Then we could just write our own tiny helper function that read from it (if that was actually needed).

(This also assumes that we just use the browser's built-in play/pause script execution functionality.

@acdlite

This comment has been minimized.

Copy link
Member

acdlite commented Nov 6, 2018

Like @bvaughn I'm unconvinced that pauseExecution and resumeExecution are necessary. What do they solve that couldn't be solved by DevTools breakpoints or debugger statements?

dumpQueue seems useful but also doesn't need to live in the Scheduler package.

@mrkev

This comment has been minimized.

Copy link
Contributor

mrkev commented Nov 6, 2018

@bvaughn @acdlite +1 to moving dumpQueue out of scheduler and exposing the firstCallbackNode, I'll make that change. I can look into exposing the same API and nooping in prod too.

On why pause, some of my thoughts:

  1. Inspecting the current queue is useful for debugging, but things could change too quickly if not paused. Breakpoints and debugger statements pause all JS execution so it's probably hard to build custom in-page tools around these. How do I update my UI?

  2. I'm thinking a useful workflow to understand and debug the scheduler might be stepping through the callbacks one at a time and seeing how the page/queue change after each one. Doing this with breakpoints would be hard since people would need to find the break on the scheduler's code, not their own. This is not something implemented in this PR, but depends on being able to soft-stop the scheduler.

If I'm missing something about how built-in tools work or useful workflows do let me know though. How does tooling around scheduler look for you guys?

Tagging @n8schloss in case he has more thoughts to add

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Nov 6, 2018

Inspecting the current queue is useful for debugging, but things could change too quickly if not paused. Breakpoints and debugger statements pause all JS execution so it's probably hard to build custom in-page tools around these. How do I update my UI?

Why can't your custom tooling just log to the console? (Why would it need an in-page UI?)

I'm thinking a useful workflow to understand and debug the scheduler might be stepping through the callbacks one at a time and seeing how the page/queue change after each one. Doing this with breakpoints would be hard since people would need to find the break on the scheduler's code, not their own. This is not something implemented in this PR, but depends on being able to soft-stop the scheduler.

Why would the ability to programmatically pause/unpause the scheduler make this any easier than just setting breakpoints (conditional if needed) in the Source panel and stepping through from there? Aren't you stepping through from a fixed point in code either way?

@mrkev

This comment has been minimized.

Copy link
Contributor

mrkev commented Nov 6, 2018

Why can't your custom tooling just log to the console?

It can. Do we not want an in-page UI?

Why would the ability to programmatically pause/unpause the scheduler make this any easier than just setting breakpoints (conditional if needed) in the Source panel and stepping through from there? Aren't you stepping through from a fixed point in code either way?

Setting breakpoints doesn't allow me to view the scheduler state after each callback execution, unless I breakpoint in the scheduler itself, which would be unnecessary complicated for most devs.

@bvaughn this is why I asked in my original comment, how does tooling for understanding/debugging scheduled code look for you guys? What's your vision here? I want to align goals here.

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Nov 7, 2018

Why can't your custom tooling just log to the console?

It can. Do we not want an in-page UI?

This PR is the first I've heard one mentioned. But insomuch as we hope for this package to eventually be adopted by browser vendors natively, I think additions like pause/continue/dump might hurt our chances of adoption. (Presumably if these APIs did become native, native browser DevTools would provide this functionality. In the meanwhile, I think it would be nice if we could get by with existing native tooling– pause/resume script execution, breakpoints, console log, etc.)

Setting breakpoints doesn't allow me to view the scheduler state after each callback execution, unless I breakpoint in the scheduler itself, which would be unnecessary complicated for most devs.

I'm not sure I understand the last bit. Most devs probably wouldn't be debugging the scheduler itself, would they? Seems like they would be setting breakpoints in their own (app or framework) code.

@n8schloss

This comment has been minimized.

Copy link
Contributor

n8schloss commented Nov 7, 2018

Why can't your custom tooling just log to the console?

It can. Do we not want an in-page UI?

This PR is the first I've heard one mentioned. But insomuch as we hope for this package to eventually be adopted by browser vendors natively, I think additions like pause/continue/dump might hurt our chances of adoption. (Presumably if these APIs did become native, native browser DevTools would provide this functionality. In the meanwhile, I think it would be nice if we could get by with existing native tooling– pause/resume script execution, breakpoints, console log, etc.)

Setting breakpoints doesn't allow me to view the scheduler state after each callback execution, unless I breakpoint in the scheduler itself, which would be unnecessary complicated for most devs.

I'm not sure I understand the last bit. Most devs probably wouldn't be debugging the scheduler itself, would they? Seems like they would be setting breakpoints in their own (app or framework) code.

So my thoughts here.

  1. In terms of browser vendor adoption, we're not asking browser vendors to standardize our implementation of scheduling, and I don't think that we'll be getting rid of the JSScheduler all together once somethings out. We're always going to have some kind of wrapper into scheduling and at that layer we can absolutely take care of pausing. So concerns about convincing browser vendors about anything here shouldn't impact if we build debugging tools or not.

  2. In terms of pausing and allowing developers to step through one function at a time, it's essential that when introducing new technologies and concepts to Facebook developers that we allow debugging tooling in order for folks (and for us) to be convinced that things are working correctly. Without some kind of debugging UI I guarantee that scheduling will get blamed for bugs it has nothing to do with. These kind of tooling can be optional, and we won't show UI by default, but for anything new it's really important that we give developers some advanced debugging methods from the start. Devs absolutely want to debug what's going on with scheduler themselves so it's something we need to support. Without being able to integrate the few changes we need into the open source repo we'd have to fork parts internally, which will end up being very messy.

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Nov 7, 2018

In terms of browser vendor adoption, we're not asking browser vendors to standardize our implementation of scheduling, and I don't think that we'll be getting rid of the JSScheduler all together once somethings out.

Please correct me if I'm wrong.

I was under the impression that we essentially were asking them to standardize what's in the "scheduler" package. The JSScheduler is a Facebook wrapper around that package, so it's reasonable to add layers to it, but this PR is proposing changes to the "scheduler" package.

In terms of pausing and allowing developers to step through one function at a time, it's essential that when introducing new technologies and concepts to Facebook developers that we allow debugging tooling in order for folks (and for us) to be convinced that things are working correctly. Without some kind of debugging UI I guarantee that scheduling will get blamed for bugs it has nothing to do with.

This sounds reasonable.

Have we considered creating a custom web extension for this purpose? (I'm not sure how this would work initially. Just thinking out loud.)

@n8schloss

This comment has been minimized.

Copy link
Contributor

n8schloss commented Nov 7, 2018

In terms of browser vendor adoption, we're not asking browser vendors to standardize our implementation of scheduling, and I don't think that we'll be getting rid of the JSScheduler all together once somethings out.

Please correct me if I'm wrong.

I was under the impression that we essentially were asking them to standardize what's in the "scheduler" package. The JSScheduler is a Facebook wrapper around that package, so it's reasonable to add layers to it, but this PR is proposing changes to the "scheduler" package.

Yes, but my point is that I think there will always be a place for some type of scheduler package even inside of React and it's there that this type of tooling should live. I do think that browsers will want to think about debugging tooling when implementing a scheduler, but that's orthogonal to this.

In terms of pausing and allowing developers to step through one function at a time, it's essential that when introducing new technologies and concepts to Facebook developers that we allow debugging tooling in order for folks (and for us) to be convinced that things are working correctly. Without some kind of debugging UI I guarantee that scheduling will get blamed for bugs it has nothing to do with.

This sounds reasonable.

Have we considered creating a custom web extension for this purpose? (I'm not sure how this would work initially. Just thinking out loud.)

Cool :) Yeah, I think an open source extension could eventually be neat here too! The extension would still need hooks into the scheduler though, so it doesn't change much for this diff imo.

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Nov 7, 2018

Yes, but my point is that I think there will always be a place for some type of scheduler package even inside of React and it's there that this type of tooling should live. I do think that browsers will want to think about debugging tooling when implementing a scheduler, but that's orthogonal to this.

Agreed. I think the concern Andrew and I had was that adding non-essential code to this package may hurt our chance for browser adoption.

Cool :) Yeah, I think an open source extension could eventually be neat here too! The extension would still need hooks into the scheduler though, so it doesn't change much for this diff imo.

Well I was asking because if there was a global var (in DEV mode) then perhaps the extension could read from it and provide its own UI to inspect the scheduled state while the scheduler was paused (using the browser's built-in pause script execution functionality).

Not sure entirely how this would work, since pausing script execution would also pause posted messages between an extension and the front-end, but...maybe it would be possible somehow.

@n8schloss

This comment has been minimized.

Copy link
Contributor

n8schloss commented Nov 7, 2018

Well I was asking because if there was a global var (in DEV mode) then perhaps the extension could read from it and provide its own UI to inspect the scheduled state while the scheduler was paused (using the browser's built-in pause script execution functionality).

Not sure entirely how this would work, since pausing script execution would also pause posted messages between an extension and the front-end, but...maybe it would be possible somehow.

Exactly :) Even in that case we're still going to need to let the extension hook into and pause the scheduler.

Anyway, I think we should add these functions in for now, and then later if we break some of the internal debugging tools it'll be fine and we'll figure out how to fix them/we're still naming these things unstable_, so they'll for sure be subject to change.

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Nov 7, 2018

Exactly :) Even in that case we're still going to need to let the extension hook into and pause the scheduler.

Why is this necessarily true? I was suggesting that we could play/pause the scheduler using the browser's built-in "pause script execution" functionality.

Let's talk about this during the sync tomorrow? This is getting to be too much back and forth. 😄

@n8schloss

This comment has been minimized.

Copy link
Contributor

n8schloss commented Nov 7, 2018

Why is this necessarily true? I was suggesting that we could play/pause the scheduler using the browser's built-in "pause script execution" functionality.

Let's talk about this during the sync tomorrow? This is getting to be too much back and forth. 😄

I'm not sure if we can do that and still get the debugger to display what we need and beyond that, it won't let us build the scheduler stepping mechanism that we want to build. So hooks into the scheduler are very important. Anyway, happy to chat about it tomorrow.

@mrkev

This comment has been minimized.

Copy link
Contributor

mrkev commented Nov 13, 2018

@acdlite @bvaughn updated with changes discussed!

@acdlite

This comment has been minimized.

Copy link
Member

acdlite commented Nov 19, 2018

Please wrap in a feature flag, then I think this is ok to land.

@acdlite acdlite force-pushed the mrkev:scheduler-debug-funcs branch from fb45f36 to 3c6bb9d Dec 6, 2018

@acdlite acdlite merged commit 8df4d59 into facebook:master Dec 6, 2018

1 check was pending

ci/circleci CircleCI is running your tests
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment