-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
a few assorted issues #81
Comments
Thanks so much for reporting these! Very helpful. I think these all seem pretty straightforward to fix, so they should be fixed soon. Just curious: how did you come across these? I was wondering if you used some automated means and/or had a test suite that caught these, since that would also be helpful for testing. Manual testing is also very much appreciated of course :) |
I've encountered and/or fixed similar corner issues in a few ES tools so I thought I'd try them out here. Take a look at terser's test suite which was derived from uglify-js. You can swap out the minify function for yours and ignore the expected generated code and just match each test's Another useful exercise I've found is to minify the generated |
Thanks for the tips! Glad I asked. I'll definitely check those out. |
When I think about it, rewriting the Both terser and uglify-js "compress" tests have valid ES syntax, but are not run directly. Here's a few examples: func_arg_1: {
options = {
evaluate: true,
inline: true,
passes: 2,
reduce_funcs: true,
reduce_vars: true,
side_effects: true,
toplevel: true,
unused: true,
}
input: {
var a = 42;
!function(a) {
console.log(a());
}(function() {
return a;
});
}
expect: {
console.log(42);
}
expect_stdout: "42"
} array_forin_1: {
options = {
reduce_funcs: true,
reduce_vars: true,
toplevel: true,
unused: true,
}
input: {
var a = [ 1, 2, 3 ];
for (var b in a)
console.log(b);
}
expect: {
for (var b in [ 1, 2, 3 ])
console.log(b);
}
expect_stdout: [
"0",
"1",
"2",
]
} iife: {
options = {
evaluate: true,
reduce_funcs: true,
reduce_vars: true,
}
input: {
!function(a, b, c) {
b++;
console.log(a - 1, b * 1, c + 2);
}(1, 2, 3);
}
expect: {
!function(a, b, c) {
b++;
console.log(0, 3, 5);
}(1, 2, 3);
}
expect_stdout: true
} You would only care about the contents of the Although uglify-js is ES5 only, it is still actively maintained and has a number of tests that terser lacks and vice-versa. |
@kzc I have some updates. Since the above posts, I have added an API called I also just landed a The test suite has lots of helpful failures that I can start to work through. Thank you so much for telling me about it. Some of them are known issues around eval/with/arguments scope stuff, and some of them are unexpected edge cases that are good to know about. I'm still going through the test failures. I also got esbuild to bundle and minify Rollup from the TypeScript source. It only required adjusting a few import paths because Rollup's build uses some custom path aliases. This exposed a single TypeScript bug which was fixed in f12f6c7. I then ran that build through Rollup's whole test suite and all tests pass. |
@evanw Glad you got the terser tests working. I thought porting the test runner was the more difficult route, but clearly that did not present any difficulty once you created the new JS API. You can repeat the test process with https://github.com/mishoo/UglifyJS/tree/master/test/compress. Although there is a lot of overlap with terser's forked tests, there are a great number of new (and subsequently altered) uglify-js ES5 tests since the terser was forked. Keep in mind that the test filenames may often be the same with different contents. |
@evanw It'd be useful to users if the timings to build Rollup from TS sources were shown compared to Rollup itself ( |
The readme now mentions that the esbuild bundle is able to successfully run the Rollup test suite. Rollup builds with esbuild in ~40ms on my machine. Rollup builds itself in ~4820ms. I'm not sure they are exactly equivalent though since the Rollup config file appears to cause other files to be generated too. I didn't take the time to understand what's going on in detail. For future reference, the full commands were: # For esbuild
esbuild --bundle src/node-entry.ts --outfile=dist/rollup.js --platform=node --target=es2018 --external:fsevents --minify
# For rollup
node_modules/.bin/rollup -c --configTest I'm not planning on including it as a benchmark in the readme though because I already have a TypeScript benchmark, and I'm trying to keep the benchmarks clean (one for each topic). The Rome code base is bigger than Rollup so I think it makes for a better TypeScript benchmark. Also esbuild can't yet build the Rollup code cleanly. I had to make a few changes to Rollup's source code to avoid the need for custom path aliases, since esbuild doesn't support them yet (see #38). So it would be more appropriate as a benchmark once esbuild has support for aliases. |
On the topic of benchmarks, the three.js test case is a good example of bundling throughput, but no code can be dropped in that scenario. By design all code in a library is retained. It would be good to also benchmark bundling an end user application like something created with react and material-ui that would better exercise selective code importing, optimization and dead code elimination. |
Yeah that'd be great. Do you know of any good sizable open source ones like that? Ideally it'd take at least >30 seconds to be built by other tools. |
That's the thing - the largest examples of source code bases are usually the bundlers. I don't know of any off the top of my head. |
@evanw Generally libraries are more likely to be open source and web applications are proprietary. But here's a good sized representative example application using react and material-ui: https://github.com/marmelab/react-admin/tree/master/examples/simple It's designed to be built with create-react-app and webpack but some quick hacking allows this self-contained example directory to be built with rollup using:
and with esbuild 0.3.9:
I can't attest to the correctness of the builds - I haven't tried running either and may have missed something. There's likely some create-react-app configuration I'm not aware of to get it working. Rollup produced two chunks, and esbuild just one. The esbuild bundle is roughly 39% larger than the aggregate size of the rollup chunks, possibly due to these factors. |
https://github.com/marmelab/react-admin/tree/master/examples/simple with esbuild 0.4.0:
|
fwiw, here's https://github.com/marmelab/react-admin/tree/master/examples/simple with esbuild 0.4.7:
It's in the same ballpark as rollup+terser:
|
Thanks! Yeah I tested that as well. I was able to get the build running in the browser too and everything seemed functional to me. |
Can you share how to run the resultant bundle in a browser? I couldn't figure it out. |
Sure:
Then open http://localhost:8000/dist/ in a browser. |
Thanks! It's the darnedest thing - I thought I tried that. Wait - I was missing In my opinion the artificial synthetic bench/three should be replaced with https://github.com/marmelab/react-admin/tree/master/examples/simple since it's more a kin to a real life web application with dead code eliminated. |
If you decide to use react-admin/examples/simple as a bench, the rollup terser plugin would be:
It requires the use of a |
@evanw There's a bundle size regression in react-admin/examples/simple from 0.5.0 forward...
0.4.7: 1368131 dist/main.js All versions appear to work correctly in the browser. |
Thanks for the heads up. It looks like the bundle size regression happened in 0.4.12 (vs 0.4.11). That release included some changes to how |
You might double check esbuild 0.5.3 against the latest version of https://github.com/mischnic/tree-shaking-example. It resolves some other bundler issues in your fork. |
I have investigated the size regression in react-admin/examples/simple. Here's what happens:
This library is especially strange because the export { default as SvgIcon } from './SvgIcon';
export * from './SvgIcon';
export { default as SwipeableDrawer } from './SwipeableDrawer';
export * from './SwipeableDrawer';
// ... many more like this ... Each file seems to just have two exports, Right now modules in esbuild are either CommonJS or not (a binary choice). I suppose I could try to address this by giving modules a third state that is both CommonJS and not CommonJS, in the sense that it must be CommonJS because code needs to
I tried running esbuild 0.5.3 and the bundle sizes didn't change. So it looks like there are no esbuild regressions in those benchmarks. I did find what seems like another issue with that repo though. Rollup generates warnings about missing exports for |
What a deep dive. I thought Just curious why esbuild 0.4.11 and earlier versions produced a smaller apparently correct working bundle.
If the modules were ultimately not needed to produce the correct But the configuration for rollup to deal with the react CJS module is indeed cumbersome and error prone. It involves a lot of user configuration that other bundlers appear to figure out on their own. This could be remedied by the react project releasing an ES module version of their library, or better heuristics in the commonjs rollup plugin. |
This is interesting. I took a deeper look and there is indeed an esm variant, but it's not getting picked. Both This is one such import chain leading to that decision: // src/Layout.js
import { Layout, AppBar, UserMenu, useLocale, useSetLocale } from 'react-admin';
// node_modules/react-admin/esm/index.js
import AdminRouter from './AdminRouter';
// node_modules/react-admin/esm/AdminRouter.js
import { Loading } from 'ra-ui-materialui';
// node_modules/ra-ui-materialui/esm/index.js
export * from './layout';
// node_modules/ra-ui-materialui/esm/layout/index.js
import UserMenu from './UserMenu';
// node_modules/ra-ui-materialui/esm/layout/UserMenu.js
import AccountCircle from '@material-ui/icons/AccountCircle';
// node_modules/@material-ui/icons/AccountCircle.js
var _createSvgIcon = _interopRequireDefault(require("./utils/createSvgIcon"));
// node_modules/@material-ui/icons/utils/createSvgIcon.js
var _SvgIcon = _interopRequireDefault(require("@material-ui/core/SvgIcon")); This switches over from ES6 to CommonJS in So it looks like this is potentially a bug in the
Before version 0.4.12, esbuild would silently ignore star re-exports from CommonJS modules and generate potentially incorrect code with missing re-exports. Starting with version 0.4.12 esbuild now generates code that is always correct¹ because it handles star re-exports from CommonJS modules. The build was still correct with the old version of esbuild because these star re-exports happened to be meaningless and didn't have an observable effect. But that's not going to be the case for other libraries so esbuild can't revert back to the old behavior of ignoring star re-exports from CommonJS modules. ¹ At least as correct as is reasonably possible without |
After some debugging, rollup appears to resolve to the same material-ui source paths that esbuild does. I don't know whether its smaller bundle size is attributable to rollup's general tree shaking heuristics, or how it implements
It does seem odd that the esm sources explicitly reference CJS source paths in the same library.
Not that I know of. What would be the heuristic? If an ES module imports a CJS module from the same library emit a warning? Or only emit if the warning if there's a plausible esm equivalent path? |
@evanw I noticed a 25% speed regression in I was curious to see how react-admin/examples/simple fares using esbuild-wasm with the commands provided in #81 (comment). Here's a baseline reference using an older version - it finishes in just under 11 seconds:
It produces a 1.4M output bundle. However, the latest version of esbuild-wasm fails after 2 minutes with an out of memory error...
|
Thanks for the heads up. That's concerning. I periodically test benchmark performance as I work to make sure there haven't been any regressions, but only on native. The WebAssembly version is already so slow (and isn't really designed to be fast anyway) that I haven't been bothering to test it as I work. Looks like I should probably start testing it periodically too. The regression was introduced between version 0.5.26 and version 0.6.0. I narrowed down the cause of the out-of-memory error to this commit: 0a8bbfd. This change moves path resolution from the main thread to the parse thread. Path resolution means going through all of the imports in a file and resolving the path text to the absolute path of the actual file on disk. I moved it from the main thread to the per-file parser thread since it's a per-file task, so it shouldn't be blocking the main thread. This will be especially important when plugins are introduced because path resolving may involve running resolver plugins, which may end up running JavaScript in the parent process via IPC. Builds will be much slower if that work is done serially on the main thread instead of in parallel for each file. However, it does mean that there will now be multiple simultaneous path resolutions going on in parallel (by design). It looks like this is having a significant effect on the behavior of the build. I haven't figured out why that is yet. |
Might c25c607 be detrimental to wasm CLI performance? |
I am thinking about that change, yeah. It could make sense to only do that in 64-bit environments in which case the WebAssembly environment would start running the GC. However, it seems intuitively like that would just make things run slower instead of faster since it's doing more work. FWIW I've already tried enabling the GC again and that avoids the OOM but it's still nowhere near as fast as it was before. The thing I don't understand yet is that the serial vs. parallel path resolution change should be doing approximately the same amount of work as before, so it shouldn't be using a lot more memory. Parallel vs. serial on a single-threaded runtime with the GC disabled should end up with around the same memory usage at the end either way I'd think. I'd like to understand why this happens and implement the correct fix instead of just making a change that hides the problem. I'm still investigating this. Will keep you posted. |
Is there a reason why you're using the WebAssembly version on the command line by the way? The native version is equivalent and faster, so I can't think of a reason to do this myself. I think the WebAssembly implementation is really only useful in the browser, where running native code isn't an option. The performance and memory issues you're calling out here are only a problem with bundling, and the bundling API isn't exposed in the browser. So these problems shouldn't come up in the normal WebAssembly use case. |
The Go runtime doesn't work on my old version of Mac OS, and I like the idea of using the same portable wasm package on all platforms. A 10 second build is adequate for what I need. |
If I had to guess not knowing much about Go or the esbuild code base, the IO bound parallel resolution within the parsing goroutines might cause them to repeatedly context switch out between each other due to the single-threaded wasm runtime producing a larger working set of memory. |
After a lot of debugging, I've discovered that part of the problem is the |
Ok, give version 0.6.25 a try. I believe I have fixed the issues in this test case. Path resolving still originates from separate threads but the resolver itself now has a single lock around it. That should allow resolver plugins to still run concurrently but the resolver fallback will now run serially. This change fixes the memory issues and is responsible for most of the performance improvement. I also am no longer eagerly calling And I discovered an additional WebAssembly-related performance improvement (in the release notes) which means the command-line WebAssembly build should now be faster than before. Version 0.5.26 (the version before the major regression) builds the react-admin benchmark in around 6.81s for me while version 0.6.25 builds it in 3.97s. I have added the react-admin benchmark to my main rotation and will now keep an eye on it as I develop. |
react-admin/examples/simple is indeed much faster in esbuild-wasm@0.6.25 - real 5.561s, user 14.003s, sys 0.575s on my machine. Likewise, The advantage of testing against a slower thread poor platform like wasm is that minute inefficiencies are exaggerated. |
Here's a how-to for the handful of people on earth using an unsupported version of Mac OSX and wish to run the native version of
This produces a shared library Set these environmental variables either in
Optional: Install the official Go release by untarring https://golang.org/dl/go1.15.darwin-amd64.tar.gz. Assuming
A couple of bugs in |
esbuild-minify
is a dumb wrapper script overesbuild --minify
- it's not important other than to demonstrate the following minification issues.Default argument scope:
Switch expression scope:
Object literal computed property output:
Class method computed property output:
The text was updated successfully, but these errors were encountered: