-
Notifications
You must be signed in to change notification settings - Fork 69
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
Add webpack benchmark #8
base: master
Are you sure you want to change the base?
Conversation
jest.config.js
Outdated
|
||
module.exports = { | ||
watchPathIgnorePatterns: [ | ||
path.resolve(__dirname, "build") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary because the webpack test needs to be built with webpack first. This watch ignore pattern fixes an infinite loop issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do this as a separate PR? Just adding the jest.config.js
file. This seems useful on it's own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to ignore that path without the webpack test because other tests are not writing any files. Personally, I wouldn't add that configuration when it's not necessary. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
@@ -79,5 +79,18 @@ fs.writeFileSync( | |||
"third_party/vue.runtime.esm-nobuble-2.4.4.js", | |||
require("raw-loader!../third_party/vue.runtime.esm-nobuble-2.4.4.js") | |||
); | |||
fs.mkdirpSync("/src/fixtures/webpack"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Webpack requires absolute file paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does that work when you run it via node
? I.e.
$ node src/cli
jest.config.js
Outdated
|
||
module.exports = { | ||
watchPathIgnorePatterns: [ | ||
path.resolve(__dirname, "build") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do this as a separate PR? Just adding the jest.config.js
file. This seems useful on it's own.
src/webpack-benchmark.js
Outdated
name: "webpack", | ||
defer: true, | ||
fn(deferred) { | ||
return Promise.all( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a problem. I carefully avoided any interaction with the microtask queue, i.e. any use of promises, because that way you no longer measure the core JavaScript bits, but all of a sudden mix in embedder tasks into the measurement (i.e. DOM tasks scheduled on the microtask queue). Also the microtask queue doesn't work the same across different shells (not even across different versions of the same shell).
Please use a synchronous API instead.
Ideally webpack (and rollup FWIW) would provide a simple API entry where you can feed in strings and it would do the tree shaking and bundling on that, and produce a single string output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that is going to be interesting 😁 maybe it's possible, but I'm not sure how deep the assumption of an async file system access is buried in the code 😁
We will also leave the regular code path, but that's probably a fair trade-off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's the only option for this, otherwise we get all kinds of other stuff into the measurement and the results will vary based on whether it's run in Node or the browser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I took a look and so far it looks promising.
Basically, we just need to provide synchronous mocks for setImmediate()
and process.nextTick()
. There are, however, assumptions about the execution order. That's why I had to implement a simple event loop. But with my event loop, it looks like webpack is compiling synchronously. The event loop looks basically like this:
const loop = [];
global.process = {
nextTick(fn) {
const args = Array.prototype.slice.call(arguments, 1);
loop.push(() => {
fn.apply(null, args);
});
}
};
global.setImmediate = function (fn) {
const args = Array.prototype.slice.call(arguments, 1);
loop.push(() => {
fn.apply(null, args);
});
};
// and then during the benchmark
let task;
while ((task = loop.shift())) {
task();
}
Do you think that would be ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting approach. Do you think we could somehow make the setImmediate
and process.nextTick
mock local to the webpack benchmark? I don't like changing global state. That can easily screw up other benchmarks w/o us noticing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we could inject our process
and setImmediate
mock to only webpack and virtualfs (virtualfs is using setImmediate
to fake async method calls).
However, I would inject these mocks to all modules. Because if any module inside the benchmark is using one of these methods, the benchmark cannot be sync by design so I'd say it's fair to mock these throughout the whole bundle.
That would also make this mocks only available to the local module scope instead of mutating global
which ensures that we don't touch the global object.
The benchmark now executes synchronously. All modules inside the bundle use our |
eventLoop.run(); | ||
|
||
if (finished !== true) { | ||
throw new Error("Webpack did not finish synchronously"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, that's a lot of magic, and the magic doesn't work for node
unfortunately:
$ node src/cli
Running Web Tooling Benchmark 0.2.0...
--------------------------------------
Encountered error running benchmark webpack, aborting...
Error: Webpack did not finish synchronously
at payloads.forEach.config (/usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/src/webpack-benchmark.js:45:15)
at Array.forEach (<anonymous>)
at Benchmark.fn (/usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/src/webpack-benchmark.js:27:14)
at Benchmark.eval (eval at createCompiled (/usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/node_modules/benchmark/benchmark.js:1725:16), <anonymous>:5:124)
at clock (/usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/node_modules/benchmark/benchmark.js:1644:22)
at clock (/usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/node_modules/benchmark/benchmark.js:1818:20)
at cycle (/usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/node_modules/benchmark/benchmark.js:2007:49)
at Benchmark.run (/usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/node_modules/benchmark/benchmark.js:2114:13)
at execute (/usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/node_modules/benchmark/benchmark.js:860:74)
at invoke (/usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/node_modules/benchmark/benchmark.js:970:20)
/usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/src/webpack-benchmark.js:34
throw err;
^
EntryModuleNotFoundError: Entry module not found: Error: Can't resolve '/src/fixtures/webpack/a.js' in '/usr/local/google/home/bmeurer/Projects/web-tooling-benchmark'
at moduleFactory.create (/usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/node_modules/webpack/lib/Compilation.js:411:30)
at factory (/usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/node_modules/webpack/lib/NormalModuleFactory.js:235:20)
at resolver (/usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/node_modules/webpack/lib/NormalModuleFactory.js:60:20)
at asyncLib.parallel (/usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/node_modules/webpack/lib/NormalModuleFactory.js:127:20)
at /usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/node_modules/async/dist/async.js:3861:9
at /usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/node_modules/async/dist/async.js:421:16
at iteratorCallback (/usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/node_modules/async/dist/async.js:996:13)
at /usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/node_modules/async/dist/async.js:906:16
at /usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/node_modules/async/dist/async.js:3858:13
at resolvers.normal.resolve (/usr/local/google/home/bmeurer/Projects/web-tooling-benchmark/node_modules/webpack/lib/NormalModuleFactory.js:119:22)
Wouldn't it be easier to just add a synchronous entry point to webpack? That seems to have benefits beyond performance testing, i.e. you could easily unit test it w/o having to deal with promises and stuff.
@@ -79,5 +79,18 @@ fs.writeFileSync( | |||
"third_party/vue.runtime.esm-nobuble-2.4.4.js", | |||
require("raw-loader!../third_party/vue.runtime.esm-nobuble-2.4.4.js") | |||
); | |||
fs.mkdirpSync("/src/fixtures/webpack"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does that work when you run it via node
? I.e.
$ node src/cli
chokidar: require.resolve("./src/mocks/chokidar"), | ||
"uglify-js": require.resolve("./src/mocks/dummy"), | ||
// These modules are used by virtualfs to fake async fs calls | ||
"core-js/library/fn/set-immediate": require.resolve( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does that work with running the benchmark suite via node
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below
The magic should work 😁 Shouldn't you execute
Seems like the |
That's exactly the thing: The benchmark should work unbundled, just by running I really appreciate your contribution and all the time and energy you put into this. But I also need to stress this point a bit, that we need to have a reliable way to just run the webpack core logic in a synchronous way. How hard would it be to modify webpack core to provide such a way? Maybe just doing that on a branch initially, which is then used for benchmarking? |
Ok, then it seems like I've misunderstood the goal of the benchmark. My understanding was that the benchmark is intended to be executable without any dependency on host objects, hence the bundling with webpack because a basic webpack bundle (without code splitting or other fancy stuff) should be executable in any modern JS environment. I didn't know that it should also be executable in Node.js without touching anything. It would be nice to have a list somewhere about which file should be executable where. Or to have an instruction how to test the benchmark.
I think I need to talk with @sokra about this. From my POV, it's hard to isolate a core logic because one important part of webpack's core logic is to discover the dependency graph via async file system calls (which also includes async resolving). So we would need to start with a pre-discovered dependency graph, which might be possible, but is a bit far away from the regular execution flow. There might be a solution in between: Webpack allows to swap out the file system. So we could use a mock file system, but it expects an async API. That's where the sync event loop would come into play. This approach should be executable in Node without bundling it. There are, however, calls of |
The main problem with all this magic is that the benchmark is now dominated by the |
Agreed; sorry we didn’t make this clear before! Now we have: https://github.com/v8/web-tooling-benchmark/blob/master/CONTRIBUTING.md#tests |
@mathiasbynens thanks. Is there also a browser list in which browsers it should work? |
All browsers, unless there's a good reason to exclude them (i.e. like old internet explorer versions don't need to be supported). But since the tools have to run on Node 6 anyways, that means you automatically support all the relevant browsers. |
c289d6a
to
164db67
Compare
This PR adds a simple webpack benchmark based on https://github.com/jhnns/webpack-browser-benchmark
During the benchmark, webpack builds a very small project with an async
import()
to also cover the code splitting part. The loader pipeline is not executed during this test as it would require a lot of more mocking. Maybe we can add that later.I don't think that bigger files as fixtures would be reasonable since webpack uses acorn internally which is already benchmarked in another test. However, we could add more files to represent a typical project. Some engine optimization probably only kick in when there are a lot of files.
There is still a problem with this PR: It does not include a polyfill forError.captureStackTrace()
so this won't work in other JS engines than the V8. How should we proceed with it? Why aren't we allowed to polyfill the global object? Maybe it is possible with webpack to inject a specialError
object so that we don't have to extend the global object, but I have to check that option...The
Error.captureStackTrace()
is not required in the code path that is executed during the benchmark. I tested it in Firefox and Safari and there were no error (and the Error object had nocaptureStackTrace
property)