DevTools: Parse named source AST in a worker

[tsirlucas]

Summary

Resolves #21855

Ended up using workerize in order to setup the worker once it allows easy imports (for babel's parse function) and exports.

Test Plan

• yarn build:chrome && yarn test:chrome --url=<URL_HERE>
• went to components tab on devtools
• checked out if the hooks names were correct
• changed the name of the hooks and tested again just in case

Also tested on Firefox, but for some reason needed to refresh the page after launching otherwise I would get a message telling me the website is not using React. Checked out on a previous commit and tested it and the same happens.

[sizebot]

Comparing: 3101872...d2b962d

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	126.81 kB	126.81 kB	=	40.60 kB	40.60 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	129.63 kB	129.63 kB	=	41.57 kB	41.57 kB
facebook-www/ReactDOM-prod.classic.js	=	403.39 kB	403.39 kB	=	74.64 kB	74.64 kB
facebook-www/ReactDOM-prod.modern.js	=	392.09 kB	392.09 kB	=	72.93 kB	72.93 kB
facebook-www/ReactDOMForked-prod.classic.js	=	403.39 kB	403.39 kB	=	74.64 kB	74.64 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against d2b962d

[bvaughn]

nit: Just to make it clearer what these two tests are focusing on, maybe we could move them down into their own describe block? e.g.

diff --git a/packages/react-devtools-extensions/src/__tests__/parseHookNames-test.js b/packages/react-devtools-extensions/src/__tests__/parseHookNames-test.js
index 56cb1f62e4..b422e82a9f 100644
--- a/packages/react-devtools-extensions/src/__tests__/parseHookNames-test.js
+++ b/packages/react-devtools-extensions/src/__tests__/parseHookNames-test.js
@@ -113,30 +113,6 @@ describe('parseHookNames', () => {
     return hookNames;
   }
 
-  it('should use worker when available', async () => {
-    const Component = require('./__source__/__untransformed__/ComponentWithUseState')
-      .Component;
-
-    window.Worker = true;
-    // resets module so mocked worker instance can be updated
-    jest.resetModules();
-    parseHookNames = require('../parseHookNames').parseHookNames;
-
-    const hookNames = await getHookNamesForComponent(Component);
-    expectHookNamesToEqual(hookNames, ['foo', 'bar', 'baz']);
-    expect(workerizedParseMock).toHaveBeenCalledTimes(3);
-  });
-
-  it('should use babel parser when worker is not available', async () => {
-    const Component = require('./__source__/__untransformed__/ComponentWithUseState')
-      .Component;
-
-    const hookNames = await getHookNamesForComponent(Component);
-    expectHookNamesToEqual(hookNames, ['foo', 'bar', 'baz']);
-    expect(workerizedParseMock).toHaveBeenCalledTimes(0);
-    expect(babelParserMock).toHaveBeenCalledTimes(3);
-  });
-
   it('should parse names for useState()', async () => {
     const Component = require('./__source__/__untransformed__/ComponentWithUseState')
       .Component;
@@ -205,6 +181,32 @@ describe('parseHookNames', () => {
 
   // TODO Test that cached metadata is purged when Fast Refresh scheduled
 
+  describe('parsing', () => {
+    it('should use worker when available', async () => {
+      const Component = require('./__source__/__untransformed__/ComponentWithUseState')
+        .Component;
+
+      window.Worker = true;
+      // resets module so mocked worker instance can be updated
+      jest.resetModules();
+      parseHookNames = require('../parseHookNames').parseHookNames;
+
+      const hookNames = await getHookNamesForComponent(Component);
+      expectHookNamesToEqual(hookNames, ['foo', 'bar', 'baz']);
+      expect(workerizedParseMock).toHaveBeenCalledTimes(3);
+    });
+
+    it('should use babel parser when worker is not available', async () => {
+      const Component = require('./__source__/__untransformed__/ComponentWithUseState')
+        .Component;
+
+      const hookNames = await getHookNamesForComponent(Component);
+      expectHookNamesToEqual(hookNames, ['foo', 'bar', 'baz']);
+      expect(workerizedParseMock).toHaveBeenCalledTimes(0);
+      expect(babelParserMock).toHaveBeenCalledTimes(3);
+    });
+  });
+
   describe('inline, external and bundle source maps', () => {
     it('should work for simple components', async () => {
       async function test(path, name = 'Component') {
diff --git a/yarn.lock b/yarn.lock
index c0bddfe82c..03daa69ff8 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -14877,6 +14877,13 @@ worker-loader@^3.0.2:
     loader-utils "^2.0.0"
     schema-utils "^2.7.0"
 
+workerize-loader@^1.3.0:
+  version "1.3.0"
+  resolved "https://registry.yarnpkg.com/workerize-loader/-/workerize-loader-1.3.0.tgz#4995cf2ff2b45dd6dc60e4411e63f5ae2c704d36"
+  integrity sha512-utWDc8K6embcICmRBUUkzanPgKBb8yM1OHfh6siZfiMsswE8wLCa9CWS+L7AARz0+Th4KH4ZySrqer/OJ9WuWw==
+  dependencies:
+    loader-utils "^2.0.0"
+
 wrap-ansi@^2.0.0:
   version "2.1.0"
   resolved "https://registry.yarnpkg.com/wrap-ansi/-/wrap-ansi-2.1.0.tgz#d8fc3d284dd05794fe84973caecdd1cf824fdd85"

[bvaughn]

Did you do any measuring to see how perf was affected by moving parsing off the main thread? (How much overhead comes from passing the source code and AST back and forth like this?)

Doing a little smoke testing, and the overall performance (on Facebook in DEV mode) doesn't seem to be much improved on this branch, I suspect b'c both the source code and the AST are large and serializing them to pass between contexts is probably expensive.

What if instead we passed only small pieces of info to the worker (e.g. the URL of the source file– which the worker could load, so we didn't have to serialize the source code) and we kept the AST itself in the worker itself and just passed it the line/column numbers and it passed back the hook name (which would also be small)?

[tsirlucas]

Didnt really test it against large files so my bad. Gonna check and address the requested changes this evening.

[bvaughn]

No worries at all! I really appreciate your help picking up this task 😄 Happy to talk more about it and help test the changes on my end.

[bvaughn]

Also don't forget to run yarn install in the root directory to update the yarn.lock file for this change.

diff --git a/yarn.lock b/yarn.lock
index c0bddfe82c..03daa69ff8 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -14877,6 +14877,13 @@ worker-loader@^3.0.2:
     loader-utils "^2.0.0"
     schema-utils "^2.7.0"
 
+workerize-loader@^1.3.0:
+  version "1.3.0"
+  resolved "https://registry.yarnpkg.com/workerize-loader/-/workerize-loader-1.3.0.tgz#4995cf2ff2b45dd6dc60e4411e63f5ae2c704d36"
+  integrity sha512-utWDc8K6embcICmRBUUkzanPgKBb8yM1OHfh6siZfiMsswE8wLCa9CWS+L7AARz0+Th4KH4ZySrqer/OJ9WuWw==
+  dependencies:
+    loader-utils "^2.0.0"
+
 wrap-ansi@^2.0.0:
   version "2.1.0"
   resolved "https://registry.yarnpkg.com/wrap-ansi/-/wrap-ansi-2.1.0.tgz#d8fc3d284dd05794fe84973caecdd1cf824fdd85"

[tsirlucas]

@bvaughn So, spent some time thinking about how to move only the necessary parts to the worker in order to keep the cache in the main thread and only send the url to the source files to the worker but noticed that in the end I would still need to send the hooksList object in order to call findHookNames.

A solution would be also keeping I think keeping findHookNames in the main thread but thats also running some computation using the source code so it had to be kept in there and once this is true, I would have to send the hooksList to the worker anyways.

So I decided to just move the whole parser into the worker. It receives the HooksTree and returns the HookNames. I don't really have a huge project to test it out so can you give it a try?

Also fixed the other things you pointed out.

[bvaughn]

This feels much more responsive after some initial testing! 😁

I'm seeing a few components throw errors (failing to parse) that I don't think were failing before though, so I'm going to dig in tomorrow morning and see.

[tsirlucas]

This feels much more responsive after some initial testing! 😁

I'm seeing a few components throw errors (failing to parse) that I don't think were failing before though, so I'm going to dig in tomorrow morning and see.

Perfect! If you can share some details on the components and errors so I can replicate things and debug would be great!

[bvaughn]

I think what's happening here is actually interesting. I think some components are timing out before the parsing is done, after your changes 😁 The cache has as built-in timeout, but before (since the work was all sync/blocking) the timer never had a chance to run. Now it does 😆 (I enabled the __DEBUG__ logging and verified that this is what was happening.)

I think the solution is to just increase that timeout. It's really just there as a last resort in case something has gone really wrong. Let's increase it?

diff --git a/packages/react-devtools-shared/src/hookNamesCache.js b/packages/react-devtools-shared/src/hookNamesCache.js
index f385c7242..070dafeb1 100644
--- a/packages/react-devtools-shared/src/hookNamesCache.js
+++ b/packages/react-devtools-shared/src/hookNamesCache.js
@@ -18,7 +18,7 @@ import type {
 } from 'react-devtools-shared/src/types';
 import type {HookSource} from 'react-debug-tools/src/ReactDebugHooks';
 
-const TIMEOUT = 5000;
+const TIMEOUT = 30000;
 
 const Pending = 0;
 const Resolved = 1;

[tsirlucas]

Now it does

@bvaughn done 😆

[bvaughn]

Chrome seems great now 👍🏼 but it looks like Firefox was broken by this change.

You can repro with:

 yarn build:firefox && yarn test:firefox --url=https://6tmfm.csb.app/

Enabling DEBUG mode might help track down where it fails too:

diff --git a/packages/react-devtools-extensions/src/parseHookNames/parseHookNames.js b/packages/react-devtools-extensions/src/parseHookNames/parseHookNames.js
index f68c37135..12ad6f76f 100644
--- a/packages/react-devtools-extensions/src/parseHookNames/parseHookNames.js
+++ b/packages/react-devtools-extensions/src/parseHookNames/parseHookNames.js
@@ -12,7 +12,7 @@ import LRU from 'lru-cache';
 import {SourceMapConsumer} from 'source-map';
 import {getHookName} from '../astUtils';
 import {areSourceMapsAppliedToErrors} from '../ErrorTester';
-import {__DEBUG__} from 'react-devtools-shared/src/constants';
+const __DEBUG__ = true;
 import {getHookSourceLocationKey} from 'react-devtools-shared/src/hookNamesCache';
 
 import type {
diff --git a/packages/react-devtools-shared/src/hookNamesCache.js b/packages/react-devtools-shared/src/hookNamesCache.js
index 070dafeb1..3e3d805cd 100644
--- a/packages/react-devtools-shared/src/hookNamesCache.js
+++ b/packages/react-devtools-shared/src/hookNamesCache.js
@@ -7,7 +7,7 @@
  * @flow
  */
 
-import {__DEBUG__} from 'react-devtools-shared/src/constants';
+const __DEBUG__ = true;
 
 import type {HooksTree} from 'react-debug-tools/src/ReactDebugHooks';
 import type {Thenable, Wakeable} from 'shared/ReactTypes';

[bvaughn]

As a worst-case fallback, we could parameterize the worker thing to only be used for Chrome and Edge (both of which work fine with this change).

[bvaughn]

Oh right. Had to make the same change in #21897

[bvaughn]

Fantastic 🙇🏼 This feels much more responsive now. Thank you for the contribution! 👍🏼