Skip to content
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

DevTools: Parse named source AST in a worker #21902

Merged
merged 10 commits into from
Jul 21, 2021
5 changes: 3 additions & 2 deletions packages/react-devtools-extensions/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
"update-mock-source-maps": "node ./src/__tests__/updateMockSourceMaps.js"
},
"devDependencies": {
"acorn-jsx": "^5.2.0",
"@babel/core": "^7.11.1",
"@babel/plugin-proposal-class-properties": "^7.10.4",
"@babel/plugin-transform-flow-strip-types": "^7.10.4",
Expand All @@ -28,6 +27,7 @@
"@babel/preset-env": "^7.11.0",
"@babel/preset-flow": "^7.10.4",
"@babel/preset-react": "^7.10.4",
"acorn-jsx": "^5.2.0",
"archiver": "^3.0.0",
"babel-core": "^7.0.0-bridge",
"babel-eslint": "^9.0.0",
Expand Down Expand Up @@ -55,7 +55,8 @@
"web-ext": "^3.0.0",
"webpack": "^4.43.0",
"webpack-cli": "^3.3.11",
"webpack-dev-server": "^3.10.3"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

"webpack-dev-server": "^3.10.3",
"workerize-loader": "^1.3.0"
},
"dependencies": {
"web-ext": "^4"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,15 @@
// This is done to control if and how the code is transformed at runtime.
// Do not declare test components within this test file as it is very fragile.

const {parse} = require('@babel/parser');
const babelParserWorker = require('../workerizedBabelParser/babelParser.worker.js');

describe('parseHookNames', () => {
let fetchMock;
let inspectHooks;
let parseHookNames;
let babelParserMock;
let workerizedParseMock;

beforeEach(() => {
jest.resetModules();
Expand All @@ -23,6 +28,25 @@ describe('parseHookNames', () => {
console.trace('source-map-support');
});

window.Worker = undefined;

babelParserMock = jest.fn(parse);
workerizedParseMock = jest.fn(babelParserWorker.workerizedParse);

jest.mock('@babel/parser', () => {
return {
__esModule: true,
parse: babelParserMock,
};
});

jest.mock('../workerizedBabelParser/babelParser.worker.js', () => {
return {
__esModule: true,
default: () => ({workerizedParse: workerizedParseMock}),
};
});

fetchMock = require('jest-fetch-mock');
fetchMock.enableMocks();

Expand Down Expand Up @@ -89,6 +113,30 @@ 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);
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

it('should parse names for useState()', async () => {
const Component = require('./__source__/__untransformed__/ComponentWithUseState')
.Component;
Expand Down
31 changes: 17 additions & 14 deletions packages/react-devtools-extensions/src/parseHookNames.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@
* @flow
*/

import {parse} from '@babel/parser';
import LRU from 'lru-cache';
import {SourceMapConsumer} from 'source-map';
import {getHookName} from './astUtils';
import {areSourceMapsAppliedToErrors} from './ErrorTester';
import {workerizedParse} from './workerizedBabelParser';
import {__DEBUG__} from 'react-devtools-shared/src/constants';
import {getHookSourceLocationKey} from 'react-devtools-shared/src/hookNamesCache';

Expand Down Expand Up @@ -471,6 +471,7 @@ function loadSourceFiles(
async function parseSourceAST(
locationKeyToHookSourceData: Map<string, HookSourceData>,
): Promise<*> {
const promises = [];
locationKeyToHookSourceData.forEach(hookSourceData => {
if (hookSourceData.originalSourceAST !== null) {
// Use cached metadata.
Expand Down Expand Up @@ -550,24 +551,26 @@ async function parseSourceAST(
const plugin =
originalSourceCode.indexOf('@flow') > 0 ? 'flow' : 'typescript';

// TODO (named hooks) Parsing should ideally be done off of the main thread.
const originalSourceAST = parse(originalSourceCode, {
const parsePromise = workerizedParse(originalSourceCode, {
sourceType: 'unambiguous',
plugins: ['jsx', plugin],
}).then(originalSourceAST => {
hookSourceData.originalSourceAST = originalSourceAST;
if (__DEBUG__) {
console.log(
`parseSourceAST() Caching source metadata for "${originalSourceURL}"`,
);
}
originalURLToMetadataCache.set(originalSourceURL, {
originalSourceAST,
originalSourceCode,
});
});
hookSourceData.originalSourceAST = originalSourceAST;
if (__DEBUG__) {
console.log(
`parseSourceAST() Caching source metadata for "${originalSourceURL}"`,
);
}
originalURLToMetadataCache.set(originalSourceURL, {
originalSourceAST,
originalSourceCode,
});

promises.push(parsePromise);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

@tsirlucas tsirlucas Jul 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
});
return Promise.resolve();
return Promise.all(promises);
}

function flattenHooksList(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import {parse} from '@babel/parser';

export function workerizedParse(...params) {
return parse(...params);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

// This file uses workerize to load ./babelParse.worker as a webworker
// and instanciates it, exposing flow typed functions that can be used
// on other files.

import {parse} from '@babel/parser';
import WorkerizedBabelParser from './babelParser.worker';

const workerizedBabelParser = window.Worker && WorkerizedBabelParser();

type Parse = typeof parse;

export const workerizedParse = async (
input: string,
options?: {
plugins: string[],
},
) => {
// Checks if worker is not available runs regular babel parse
if (workerizedBabelParser) {
const workerParse: Parse = workerizedBabelParser.workerizedParse;
return workerParse(input, options);
}
return parse(input, options);
};
5 changes: 5 additions & 0 deletions packages/react-devtools-extensions/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ module.exports = {
},
],
},
{
test: /\.worker\.js$/,
// inline: true due to limitations with extensions
use: {loader: 'workerize-loader', options: {inline: true}},
},
],
},
};