Scheduling Profiler: Move preprocessing to web worker and add loading indicator #19759

taneliang · 2020-09-03T13:18:11Z

Builds on #19707 to further improve the UI/UX by:

Moving our expensive preprocessData call into a new web worker. This previously caused the UI to freeze for a significant amount of time (the facebook.com we have freezes my browser for >10 seconds).
Add a loading indicator. This ProcessingData component is copied from the React DevTools Profiler.

Before: freezes, errors in modal	After: loading text + no freezing, full page errors

cc @bvaughn, @kartikcho

Implementation notes

I've used Suspense to implement the loading indicator stuff. This is the first time I'm using Suspense to do anything real, so please let me know if I'm not doing anything right 😄
I've used the Resource implementation in react-devtools-shared without forking it.

UI notes

It's possible to load a second profile while the first one is still loading. I think this is fine from both UI and technical perspectives: users may want to select another profile if they selected the wrong profile/the profile is taking too long to load, and our implementation only displays the latest selected profile.

This is what it looks like:

Here's what's happening in the gif:

Load large facebook.com profile.
While it's loading, I loaded a smaller profile that finished loading and was displayed.
I also loaded another profile that caused an error.
I then waited a few seconds for the facebook.com profile to finish loading (verified afterwards, where the flamechart shows that the loading completed). Verified that the facebook.com profile's data was not displayed.
We then wait for Chrome DevTools to process the performance profile.
Chrome DevTools' Performance tool shows that multiple workers were created and processed their profiles in parallel.

codesandbox-ci · 2020-09-03T13:21:16Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit cd1f3c4:

Sandbox	Source
React	Configuration

sizebot · 2020-09-03T13:23:42Z

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against cd1f3c4

sizebot · 2020-09-03T13:28:07Z

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against cd1f3c4

taneliang · 2020-09-03T14:37:13Z

Looks like the error handling is now broken with fast refresh.

I haven't managed to find the cause yet, but here are some guesses:

I may not be using error boundaries correctly. The error boundary is catching the thrown error though. Maybe this is just not a good use case for error boundaries?
The error overlay may just be too eager.

Any idea, @bvaughn? I can look into this further tomorrow too.

bvaughn · 2020-09-03T14:43:40Z

Likely won't have time to look into this today.

taneliang · 2020-09-03T14:45:06Z

👍 No problem! I'll continue poking tomorrow.

Looks like this was not a good fit for error boundaries. See: facebook/create-react-app#6530 (comment)

taneliang

Okay, it's fixed! Looks like it just wasn't a good use case for error boundaries.

This is ready for review, @bvaughn :)

packages/react-devtools-scheduling-profiler/src/import-worker/import.worker.js

packages/react-devtools-scheduling-profiler/src/hooks.js

taneliang · 2020-09-04T13:52:51Z

packages/react-devtools-scheduling-profiler/src/SchedulingProfiler.js

-  const [profilerData, setProfilerData] = useState<ReactProfilerData | null>(
-    null,
-  );
+type DataResource = Resource<void, File, ReactProfilerData | Error>;


Also not sure if storing data + import errors in a resource is the cleanest approach. I'm doing this to handle these expected/intentional errors without throwing them and using error boundaries. Any idea how I can improve this?

bvaughn · 2020-09-04T14:23:10Z

I think this change is probably a good one, because freezing the browser when uploading a larger profile is not a good user experience, but it makes the overall upload process noticeably slower.

For example, I tested with a 95MB profile from Facebook (the largest one I had handy) and the existing app took about 1.6 seconds:

I used the same profile on (a production build of) this branch and it took about 10 seconds, most of which was in the worker:

But a decent chunk was still on main:

How big is the profile you're testing with? What do the overall load durations look like for you?

bvaughn · 2020-09-04T14:26:44Z

Something else occurs to me that I don't know the answer for: Will this change make merging the new profiler into DevTools harder? I think extensions support workers but I've never done it before.

bvaughn

Going to hold off on merging this until you've had a chance to respond to the previous two messages, but overall this looks good to me 👍

packages/react-devtools-scheduling-profiler/src/hooks.js

taneliang · 2020-09-04T14:41:41Z

the existing app took about 1.6 seconds ... this branch and it took about 10 seconds

Ouch!

I've just tried using the 57MB facebook.com profile that you sent us, and it seems like there's a difference between the project from the fellowship and what we currently have on master. I'm not seeing any noticeable difference between master and this branch though. Which existing app are you referring to?

On https://react-scheduling-profiler.vercel.app/ (currently still deployed from the MLH Fellowship repo), I'm getting ~5 seconds

On a prod build of the master branch, I'm getting ~12 seconds. Maybe something (Speedscope probably?) got deopted?

On a prod build of this branch, I'm getting the same ~11 seconds

bvaughn · 2020-09-04T14:42:49Z

Oh, interesting. I was just testing against react-scheduling-profiler.vercel.app rather than switching between master and rebuilding.

Would be good for us to follow up and figure out what deopted us, but I guess that shouldn't hold up this merge then.

taneliang · 2020-09-04T14:54:12Z

Yeah, I'll look into this further next week.

Will this change make merging the new profiler into DevTools harder? I think extensions support workers but I've never done it before.

I don't know either 😆 I did think about this, and I figured that since there's very little code needed to use web workers it shouldn't be a big issue when integrating this into DevTools. In the worst case, we can probably just remove the web worker entirely.

bvaughn · 2020-09-04T14:56:52Z

Sounds good.

Let's merge this then for now, to keep the train rolling. We'll dig in more later.

… indicator (facebook#19759) * Move preprocessData into a web worker * Add UI feedback for loading/import error states * Terminate worker when done handling profile * Add display density CSS variables

facebook-github-bot added the CLA Signed label Sep 3, 2020

taneliang marked this pull request as draft September 3, 2020 14:02

taneliang force-pushed the web-worker branch from b3e8313 to 283bc76 Compare September 3, 2020 14:27

taneliang added 5 commits September 4, 2020 11:56

Move preprocessData into a web worker

915b051

Add UI feedback for loading/import error states

cb5fed0

Terminate worker when done handling profile

3a54937

Add display density CSS variables

842af41

Replace error boundary with manual error handling

cd1f3c4

Looks like this was not a good fit for error boundaries. See: facebook/create-react-app#6530 (comment)

taneliang force-pushed the web-worker branch from 283bc76 to cd1f3c4 Compare September 4, 2020 13:34

taneliang commented Sep 4, 2020

View reviewed changes

packages/react-devtools-scheduling-profiler/src/import-worker/import.worker.js Show resolved Hide resolved

packages/react-devtools-scheduling-profiler/src/hooks.js Show resolved Hide resolved

taneliang marked this pull request as ready for review September 4, 2020 13:50

taneliang commented Sep 4, 2020

View reviewed changes

bvaughn approved these changes Sep 4, 2020

View reviewed changes

packages/react-devtools-scheduling-profiler/src/hooks.js Show resolved Hide resolved

bvaughn merged commit eabd18c into facebook:master Sep 4, 2020

taneliang deleted the web-worker branch September 4, 2020 14:57

taneliang mentioned this pull request Sep 21, 2020

Scheduling Profiler: Improve import performance by dropping IE support #19872

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scheduling Profiler: Move preprocessing to web worker and add loading indicator #19759

Scheduling Profiler: Move preprocessing to web worker and add loading indicator #19759

taneliang commented Sep 3, 2020 •

edited

Loading

codesandbox-ci bot commented Sep 3, 2020 •

edited

Loading

sizebot commented Sep 3, 2020 •

edited

Loading

sizebot commented Sep 3, 2020 •

edited

Loading

taneliang commented Sep 3, 2020

bvaughn commented Sep 3, 2020

taneliang commented Sep 3, 2020

taneliang left a comment

taneliang Sep 4, 2020

bvaughn commented Sep 4, 2020

bvaughn commented Sep 4, 2020 •

edited

Loading

bvaughn left a comment •

edited

Loading

taneliang commented Sep 4, 2020

bvaughn commented Sep 4, 2020 •

edited

Loading

taneliang commented Sep 4, 2020

bvaughn commented Sep 4, 2020

Scheduling Profiler: Move preprocessing to web worker and add loading indicator #19759

Scheduling Profiler: Move preprocessing to web worker and add loading indicator #19759

Conversation

taneliang commented Sep 3, 2020 • edited Loading

Implementation notes

UI notes

codesandbox-ci bot commented Sep 3, 2020 • edited Loading

sizebot commented Sep 3, 2020 • edited Loading

Size changes (stable)

sizebot commented Sep 3, 2020 • edited Loading

Size changes (experimental)

taneliang commented Sep 3, 2020

bvaughn commented Sep 3, 2020

taneliang commented Sep 3, 2020

taneliang left a comment

Choose a reason for hiding this comment

taneliang Sep 4, 2020

Choose a reason for hiding this comment

bvaughn commented Sep 4, 2020

bvaughn commented Sep 4, 2020 • edited Loading

bvaughn left a comment • edited Loading

Choose a reason for hiding this comment

taneliang commented Sep 4, 2020

bvaughn commented Sep 4, 2020 • edited Loading

taneliang commented Sep 4, 2020

bvaughn commented Sep 4, 2020

taneliang commented Sep 3, 2020 •

edited

Loading

codesandbox-ci bot commented Sep 3, 2020 •

edited

Loading

sizebot commented Sep 3, 2020 •

edited

Loading

sizebot commented Sep 3, 2020 •

edited

Loading

bvaughn commented Sep 4, 2020 •

edited

Loading

bvaughn left a comment •

edited

Loading

bvaughn commented Sep 4, 2020 •

edited

Loading