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

Add std/esm benchmark. #33

Closed
wants to merge 1 commit into from
Closed

Add std/esm benchmark. #33

wants to merge 1 commit into from

Conversation

jdalton
Copy link

@jdalton jdalton commented Feb 8, 2018

This adds a basic @std/esm benchmark of loading 643 lodash-es modules. The @std/esm module loader will be used to power the next major release of lodash.

It has a bunch of moving parts. On first load it does a parse using a highly tweaked version of acorn. On subsequent loads it simulates the ESM runtime with its own module loading pipeline.

This test is for the cli runs and should be skipped for browser runs.

Copy link
Member

@bmeurer bmeurer left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. There's one thing I'm wondering about though: Does @std/esm count as a web developer tool? And does it even make sense to consider outside of Node?

cc @mathiasbynens

@@ -34,6 +34,7 @@
"license": "BSD-3-Clause",
"dependencies": {
"@babel/standalone": "7.0.0-beta.32",
"@std/esm": "^0.21.1",
Copy link
Member

Choose a reason for hiding this comment

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

We use exact dependencies.

Copy link
Author

@jdalton jdalton Feb 9, 2018

Choose a reason for hiding this comment

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

Thanks for the pull request. There's one thing I'm wondering about though: Does @std/esm count as a web developer tool? And does it even make sense to consider outside of Node?

Not outside Node no. It's Node for sure. Is there a more appropriate suite?

The suite description states

This is a benchmark suite designed to measure the JavaScript related workloads commonly used by Web Developers nowadays, for example the core workloads in popular tools like Babel or TypeScript.

Lodash is on more than 2 million websites and impacts over 150,000 npm packages (many of those used by web devs). Since this will be used to load it and others, in the Node context, it seems like a good fit.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, good point. I do think we should have a lodash benchmark in there for sure. lodash is one of the most popular npm (frontend) dependencies (if not the most popular).

As for @std/esm, we should probably create a Node tooling benchmark for Node only/mostly packages. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Do you have a good lodash benchmark suite already?

Copy link
Author

@jdalton jdalton Feb 9, 2018

Choose a reason for hiding this comment

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

Do you have a good lodash benchmark suite already?

I have a pretty robust one. It allows swapping out different versions of Lodash/Underscore. It's a long running benchmark though designed to ensure the things Lodash is good at stay good and the things it's bad at stay reasonable.

As for @std/esm, we should probably create a Node tooling benchmark for Node only/mostly packages. WDYT

That's cool too though so testing the Node flavors of Babel, Webpack, etc.?

Copy link
Member

Choose a reason for hiding this comment

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

I'm very interested in the lodash benchmark. As long as the runtime is roughly comparable to the other tests it should be fine, and if not we could reduce the iteration count a bit I guess.

That's cool too though so testing the Node flavors of Babel, Webpack, etc.?

I don't think it makes sense to have webpack there. This is definitely something that should be owned and operated by the benchmarking WG. Maybe you can join the call on Monday to discuss the idea?

Copy link
Author

Choose a reason for hiding this comment

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

As long as the runtime is roughly comparable to the other tests it should be fine, and if not we could reduce the iteration count a bit I guess.

It'd need some tweaking it's a pretty massive suite.

This is definitely something that should be owned and operated by the benchmarking WG. Maybe you can join the call on Monday to discuss the idea?

Sure thing! You can DM details if you'd like.

Copy link
Member

Choose a reason for hiding this comment

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

It'd need some tweaking it's a pretty massive suite.

It also seems to mess with the global state of benchmark.js, which needs to be fixed.

Sure thing! You can DM details if you'd like.

Added note to nodejs/benchmarking#198 for monday's meeting.

@@ -45,6 +46,7 @@
"esprima": "4.0.0",
"jshint": "2.9.5",
"lebab": "2.7.7",
"lodash-es": "^4.17.5",
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Do we even need the explicit lodash-es dependency?

Copy link
Author

Choose a reason for hiding this comment

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

Would committing the entire package to the vendor folder be a better place?

@@ -0,0 +1,3 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

This file is not bundled with the webpack build? Can we avoid external configuration files?

Copy link
Author

Choose a reason for hiding this comment

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

Yep, I can remove it and specify options via its API.

@jdalton
Copy link
Author

jdalton commented Feb 9, 2018

Cool. I'll close this then to go down the WG route.

@jdalton jdalton closed this Feb 9, 2018
@jdalton jdalton deleted the esm branch February 9, 2018 09:16
@bmeurer
Copy link
Member

bmeurer commented Feb 9, 2018

Awesome, thanks. Looking forward to a lodash pull request 👍

@mathiasbynens
Copy link
Member

I’d love to hear more about why you think adding a lodash benchmark to WTB makes sense.

lodash is an immensely popular and important project for the ecosystem. It’s not, however, a “tool” that’s used in build scripts (at least not directly) in the same way that acorn, babel, babylon, buble, chai, coffeescript, espree, esprima, jshint, lebab, prepack, prettier, source-map, typescript, uglify-es, uglify-js are.

lodash fits better in the category of frameworks/libraries IMHO, and therefore I’m thinking perhaps Speedometer would be a more appropriate benchmark in which to include lodash. WDYT?

I worry that adding a lodash benchmark to WTB would not necessarily represent its real-world usage in web developer tools — it would be more akin to a synthetic benchmark. Am I looking at this the wrong way?

@jdalton
Copy link
Author

jdalton commented Feb 9, 2018

Awesome, thanks. Looking forward to a lodash pull request 👍

Okay, will do too!

@bmeurer
Copy link
Member

bmeurer commented Feb 9, 2018

I was thinking of lodash as a tool in the developers toolbox, so from that point of view it made sense to include it here. But I see your point that it'd be a synthetic benchmark if include lodash itself. It's definitely more on the framework/library side. Having a tool in the benchmark suite that's heavily based on lodash makes sense, though. Is there one?

Besides that, I guess lodash should be in Speedometer then. IIRC there's already one or two tests that make heavy use of underscore.

@bmeurer
Copy link
Member

bmeurer commented Feb 26, 2018

Resolution from benchmarking WG meeting: We will create a separate benchmark suite with the Node specific tools/libraries/loaders/runtimes, and it'll live in https://github.com/nodejs/benchmarking. @jdalton signed up for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants