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

Mini-RFC: --no-pack flag #691

Closed
ashleygwilliams opened this issue Jul 23, 2019 · 24 comments · Fixed by #695 or #1291
Closed

Mini-RFC: --no-pack flag #691

ashleygwilliams opened this issue Jul 23, 2019 · 24 comments · Fixed by #695 or #1291
Assignees
Labels
current release current todo items feature request PR attached there's a PR open for this issue

Comments

@ashleygwilliams
Copy link
Member

ashleygwilliams commented Jul 23, 2019

Feature Description
when calling wasm-pack build a user can optionally pass --no-pack and wasm-pack will build your wasm, generate js, and not build a package.json

Example

wasm-pack build --target no-modules --no-pack

Outstanding Questions

@ashleygwilliams
Copy link
Member Author

@alexcrichton
Copy link
Contributor

@Pauan can you elaborate a bit more on the motivation here as well? Are you worried about some diagnostics coming out about package.json that may be easily run into when they shouldn't?

I personally would agree that we should keep the same output folder, although it seems reasonable to restructure it slightly. For example we may not need to add a .gitignore, but just check if the top-level .gitignore ignores pkg (or something like that)

@Pauan
Copy link
Contributor

Pauan commented Jul 24, 2019

@ashleygwilliams Mode? should we stick this in as a mode?

I don't think it should use --mode, since that's orthogonal.

As for whether it should have a "mode style" flag or not, personally, I think --no-pack or --no-package is fine, but I don't have strong feelings about it.

the default outdir is called pkg.. i'd... probably keep this the default for --no-pack

I agree that pkg should still be the default. It can be changed with --out-dir, so that's not an issue.


@alexcrichton can you elaborate a bit more on the motivation here as well? Are you worried about some diagnostics coming out about package.json that may be easily run into when they shouldn't?

The motivations are:

  • Avoid doing unnecessary checks and unnecessary work.

    As an example, it wouldn't need to handle npm packages at all, it wouldn't need to merge package.json, it wouldn't need to output the extra files, etc. This improves performance.

  • Avoid unnecessary warnings (e.g. warnings for missing README, warnings for missing LICENSE, etc.)

For example we may not need to add a .gitignore, but just check if the top-level .gitignore ignores pkg (or something like that)

After looking into it, I don't have much of an opinion about .gitignore: it's a small file that just contains *, and it doesn't require much extra work, so I'm fine with continuing to output it.

@Pauan
Copy link
Contributor

Pauan commented Jul 24, 2019

This is slightly off-topic, but we should also start thinking about how to handle npm dependencies with applications.

Right now wasm-pack generates a pkg/package.json file which contains the "dependencies" for all of the Rust crates (well, at least that's the intention).

That works fine if you just publish that package.json, but not if you are creating an application (such as with the rust-webpack-template).

The issue is that there are now two package.jsons: the top-level package.json for the app, and the pkg/package.json which contains the Rust dependencies.

That means that the pkg/package.json dependencies aren't installed, so the developer has to manually install them.

So we need to somehow fix that so the pkg/package.json dependencies are automatically installed.

This is another reason for the --no-package flag: since we will need to handle npm dependencies differently, it makes sense that --no-package should trigger that special handling.

This should probably be filed as a different issue, but it is related to --no-package.

Come to think of it, given the above, maybe we should call the flag --application instead, that seems closer to the intent.

@ashleygwilliams ashleygwilliams added PR attached there's a PR open for this issue and removed PR attached there's a PR open for this issue labels Jul 24, 2019
@ashleygwilliams ashleygwilliams self-assigned this Jul 24, 2019
@alexcrichton
Copy link
Contributor

I don't really personally buy the argument about performance in the sense that you're not wrong but we're talking about a handful of milliseconds at most here. I do think it's compelling to cater to the use case of removing extraneous warnings or extraneous checks that users could stumble over (such as generating an error or a warning that doesn't make sense in the "no pack" context).

I'm wondering if we could perhaps change a few defaults and basically cater to your use case @Pauan. I suspect we could continue to just go ahead and generate package.json by default (with a flag to turn it off), and wasm-pack would likely be already adequate minus it generating an extra file.

@Pauan
Copy link
Contributor

Pauan commented Jul 25, 2019

I don't really personally buy the argument about performance in the sense that you're not wrong but we're talking about a handful of milliseconds at most here.

I have tried modifying my loader to use wasm-pack, and it has a significant performance cost:

  • with wasm-pack: 2100ms
  • with wasm-bindgen only: 1630ms

The above numbers were taken on a tiny "hello world" project which had already been compiled, so cargo build only took 130ms, which means most of the time is spent in wasm-bindgen (1500ms), followed by 470ms spent in wasm-pack.

The above is only for a single crate. It will naturally scale linearly with the number of crates. I have some projects which have 6 crates, so I expect wasm-pack will take 12600ms compared to 9780ms for wasm-bindgen, a difference of 2820ms.

Now, you might be right that the package.json handling itself isn't slow (I have to do some profiling to find out), but it seems to me that all of the checks and processing that wasm-pack does is indeed slow, and will only get slower in the future (when #606 is implemented).

An individual operation might only take a handful of milliseconds (a single disk I/O takes several ms), but when you're doing dozens or hundreds of operations, it adds up.

I suspect we could continue to just go ahead and generate package.json by default (with a flag to turn it off), and wasm-pack would likely be already adequate minus it generating an extra file.

I'm confused: my argument is that there should be a flag to prevent it from generating package.json, and you argue against that, but then you say that there should be a flag to prevent it from generating package.json?

@alexcrichton
Copy link
Contributor

Er ok so lemme explain a bit on the performance part. I've worked extensively on Cargo, rustc, etc, and I've worked quite a lot on those with respect to performance. When I say wasm-pack should take a handful of milliseconds, I mean it. I know disk I/O can be slow but the fact of the matter is that it basically isn't. Cargo does orders of magnitude more I/O than wasm-pack and wasm-bindgen (like, multiple orders), so there's absolutely no reason that wasm-pack/wasm-bindgen should be slow.

If you're seeing 1500ms in wasm-bindgen for a hello world crate, that's bad! If you're seeing a 500ms performance overhead using wasm-pack, that's bad! What I'm asserting above is obviously modulo bugs and such, but I really want to drive home that this is something we care about and a 500ms performance difference is not acceptable. It's something that needs to be diagnosed, fixed, and upstreamed.

Can you provide a sample project with reproduction steps to see the discrepancy? If wasm-bindgen takes 1500ms that almost sounds like you're doing a debug build. If wasm-pack takes 500ms extra then I have no idea where that time is going but we need to profile, figure out where it's going, and figure out how to solve it.

These are all surmountable issues and we have so much to benefit by staying consistent on the same toolchain (e.g. like Cargo for Rust) so we need to not be distracted by small bugs which make it seem like the whole toolchain needs to be updated.

I'm confused: my argument is that there should be a flag to prevent it from generating package.json, and you argue against that, but then you say that there should be a flag to prevent it from generating package.json?

Sorry what I mean is that I don't believe there's a reason to avoid generating package.json for performance. If performance is the only reason, there's no reason that flag should exist. For functionality, however, it seems like a reasonable thing to add.

@ashleygwilliams ashleygwilliams added the PR attached there's a PR open for this issue label Jul 25, 2019
@ashleygwilliams ashleygwilliams added this to the 0.9.0 milestone Jul 25, 2019
@ashleygwilliams ashleygwilliams added the current release current todo items label Jul 25, 2019
@Pauan
Copy link
Contributor

Pauan commented Jul 26, 2019

I really want to drive home that this is something we care about and a 500ms performance difference is not acceptable. It's something that needs to be diagnosed, fixed, and upstreamed.

Great to hear! I agree that if the performance issues can be fixed, then that is ideal.

Can you provide a sample project with reproduction steps to see the discrepancy? If wasm-bindgen takes 1500ms that almost sounds like you're doing a debug build.

Hold on, I need to put it somewhere public, right now it's just sitting on my harddrive.

It is doing a --release build, I never do debug builds when testing anything performance or size related.

we need to profile, figure out where it's going, and figure out how to solve it.

Yes, that was my next plan.

we need to not be distracted by small bugs which make it seem like the whole toolchain needs to be updated

Sure, part of the reason for this feature request was so that I would be able to use wasm-pack instead of wasm-bindgen-cli.

If performance is the only reason, there's no reason that flag should exist.

Ah, okay, I understand now. Assuming the performance issues can be fixed, then I agree.

But performance isn't the only reason for the flag: the handling of npm dependencies for applications is another. I think we should explore that (though it's not a blocker for me using wasm-pack).

@Pauan
Copy link
Contributor

Pauan commented Jul 26, 2019

Okay, I created a time test repo: https://github.com/Pauan/wasm-pack-perf-test

After cloning it, do yarn install and then yarn run time. I get these results:

- cargo build  : 221.22448 ms
- wasm-pack    : 703.04304 ms
- wasm-bindgen : 244.175827 ms

So wasm-bindgen is only adding ~23ms over cargo build, which isn't bad at all.

But wasm-pack is adding ~459ms over wasm-bindgen, which is quite bad.

Now I'll do some profiling to see where the issue is.

@Pauan
Copy link
Contributor

Pauan commented Jul 26, 2019

I tried turning off the build steps and turning them on one at a time, and this is the result:

no steps             = ~208ms
check_rustc_version  = ~106ms
check_crate_config   = ~128ms
build_wasm           = ~237ms
install_wasm_bindgen =  ~34ms
run_wasm_bindgen     =  ~15ms
run_wasm_opt         =  ~33ms

(I didn't list some build steps, because they took a negligible amount of time, less than 1ms)

Even when it's not running any build steps at all, it still takes ~208ms.

Checking the rustc version takes an additional ~106ms, and checking the crate config takes an additional ~128ms on top of that.

Building wasm takes ~237ms, which is to be expected (as shown in my previous post). And the rest do take up some time, but only ~88ms in total.

So now I need to figure out why checking the rustc version and crate config take so long, and why wasm-pack takes ~208ms even when it does nothing at all.

@Pauan
Copy link
Contributor

Pauan commented Jul 26, 2019

I found that cargo metadata --format-version 1 --no-deps is taking up a whopping 100ms, even though it's only outputting a small amount of JSON:

{
    "packages": [{
        "name": "wasm-pack-perf-test",
        "version": "0.1.0",
        "id": "wasm-pack-perf-test 0.1.0 (path+file:///C:/Users/Pauan/Shared%20Folders/NixOS/wasm-pack-perf-test)",
        "license": null,
        "license_file": null,
        "description": null,
        "source": null,
        "dependencies": [{
            "name": "wasm-bindgen",
            "source": "registry+https://github.com/rust-lang/crates.io-index",
            "req": "^0.2.48",
            "kind": null,
            "rename": null,
            "optional": false,
            "uses_default_features": true,
            "features": [],
            "target": null,
            "registry": null
        }, {
            "name": "web-sys",
            "source": "registry+https://github.com/rust-lang/crates.io-index",
            "req": "^0.3.25",
            "kind": null,
            "rename": null,
            "optional": false,
            "uses_default_features": true,
            "features": ["console"],
            "target": null,
            "registry": null
        }],
        "targets": [{
            "kind": ["cdylib"],
            "crate_types": ["cdylib"],
            "name": "wasm-pack-perf-test",
            "src_path": "C:\\Users\\Pauan\\Shared Folders\\NixOS\\wasm-pack-perf-test\\src\\lib.rs",
            "edition": "2018"
        }],
        "features": {},
        "manifest_path": "C:\\Users\\Pauan\\Shared Folders\\NixOS\\wasm-pack-perf-test\\Cargo.toml",
        "metadata": null,
        "authors": ["Pauan <pcxunlimited@gmail.com>"],
        "categories": [],
        "keywords": [],
        "readme": null,
        "repository": null,
        "edition": "2018",
        "links": null
    }],
    "workspace_members": ["wasm-pack-perf-test 0.1.0 (path+file:///C:/Users/Pauan/Shared%20Folders/NixOS/wasm-pack-perf-test)"],
    "resolve": null,
    "target_directory": "C:\\Users\\Pauan\\Shared Folders\\NixOS\\wasm-pack-perf-test\\target",
    "version": 1,
    "workspace_root": "C:\\Users\\Pauan\\Shared Folders\\NixOS\\wasm-pack-perf-test"
}

@Pauan
Copy link
Contributor

Pauan commented Jul 26, 2019

I believe we only really need cargo metadata to find the target_directory and the workspace_root, the rest we can get from just parsing Cargo.toml (which it already does anyways).

I also found that rustc --print sysroot takes an additional ~100ms. So between these two commands that's already ~200ms.

Even rustc --version takes ~100ms, so it sounds like there's something really wrong going on here, either with the Rust tools or with my system.

@alexcrichton
Copy link
Contributor

Hm yeah so I'm trying to get reproducible timings locally for me and what I get get with your time script is on Linux:

- cargo build  : 46.175042999999995 ms
- wasm-pack    : 199.431287 ms
- wasm-bindgen : 71.298745 ms

on macOS:

- cargo build  : 77.952 ms
- wasm-pack    : 159.001 ms
- wasm-bindgen : 78.443 ms

and on Windows

- cargo build  : 101.396599 ms
- wasm-pack    : 231.66459999999998 ms
- wasm-bindgen : 120.255201 ms

Overall except on Linux wasm-pack has a miniscule overhead (and times are relatively variable), and even on Linux it's a pretty small overhead.

Note that Cargo has mechanisms for caching rustc --print sysroot and we could basically try similar mechanisms with wasm-pack as well. For example wasm-pack probably doesn't need to check more than once a day per system that you've got wasm32-unknown-unknown installed.

If cargo metadata is taking so long that's also pretty worrisome. Long-term we could switch wasm-pack to invoke Cargo with JSON messages and rerender them in wasm-pack, but that's still aways out.

Overall it does seem like you have a particularly slow system unfortunately. What OS are you on? Do you know if there's like antivirus software or something like that which would slow things down?

@fitzgen
Copy link
Member

fitzgen commented Jul 26, 2019

I ran wasm-pack build --target web in dodrio's Todo MVC example once to get a warm cache, and then profiled running it again to measure just the overhead of a no-op build. It took about 1.4 seconds, and perf breaks it down like this:

$ perf report --sort comm --stdio --max-stack 0
[...]
# Children      Self  Command     
# ........  ........  ............
#
    48.82%    48.82%  wasm-opt    
    47.27%    47.27%  wasm-bindgen
     2.71%     2.71%  cargo       
     0.61%     0.61%  rustc       
     0.60%     0.60%  wasm-pack   

I don't think we want to turn it off by default, but I could imagine that not running wasm-opt could be useful in development scenarios.

I think it makes sense to invest some time looking into wasm-bindgen CLI's performance.

wasm-pack is not a contributor to overhead, at least on my machine and test case.

@Pauan
Copy link
Contributor

Pauan commented Jul 27, 2019

@alexcrichton Overall it does seem like you have a particularly slow system unfortunately.

But everything except the Rust tooling is fast: it's extremely weird that running wasm-bindgen --version takes 18ms, but running rustc --version takes 100ms. That doesn't sound like a "generally slow system" problem.

What OS are you on?

Windows 10 Home 64-bit (version 1803)

Do you know if there's like antivirus software or something like that which would slow things down?

A while ago I had completely turned off all anti-virus, including the built-in Windows Defender anti-virus, so that isn't the issue.


@fitzgen I'm curious what times you get when running my perf test. I just updated it to include the timings for various simple commands, this is my result:

- cargo metadata : 101.264974 ms
- rustc sysroot : 103.61345899999999 ms
- rustc version : 95.10274899999999 ms
- wasm-bindgen version : 18.190192 ms
- wasm-pack version : 22.479826 ms

- cargo build  : 222.58652999999998 ms
- wasm-pack    : 731.461546 ms
- wasm-bindgen : 254.56423199999998 ms

@nic-hartley
Copy link

This option would make wasm-pack usable for me.

I'm trying to build a site which I can deploy with GitHub Pages. The current, unconfigurable, behavior is to deploy a whole lot of extra, unnecessary stuff, including a .gitignore which ignores everything in the directory, which completely breaks every single git-push-based workflow.

In contrast, if --no-pack was provided, presumably this would exclude the unnecessary, overzealous gitignore, as well as all the other unnecessary files (the README and package.json), while still outputting the WASM, JS glue, and TypeScript definitions. This would now work with GitHub Pages.

@dakom

This comment was marked as abuse.

@alexcrichton
Copy link
Contributor

@Pauan I'm curious, can you try running the non-rustup wrappers and see if rustup is where the slowdown is being introduced? You'll have to poke around to find the binaries themselves but I'm basically curious what the difference between these two are:

$ time rustc --version
$ time `rustup which rustc` --version

@nic-hartley @dakom I think y'all's use cases are covered by the functionality changes that --no-pack is being proposed (and PR'd at #695). The discussion w/ @Pauan is largely about performance, which I think is orthogonal to the --no-pack flag itself.

@Pauan
Copy link
Contributor

Pauan commented Jul 29, 2019

@alexcrichton Yeah, it's a lot faster when using the raw path:

- rustc version : 89.755366 ms
- rustc version (raw) : 24.897281 ms
- wasm-bindgen version : 18.166887 ms
- wasm-pack version : 18.284311 ms

Using the raw path to the rustc binary isn't quite as fast as wasm-bindgen --version, but it's close, and it's ~3.5x faster than normal rustc --version.

If this discrepancy were fixed, I predict it would improve wasm-pack's performance by ~256ms, which is quite significant.

@dakom

This comment was marked as abuse.

@fitzgen
Copy link
Member

fitzgen commented Jul 29, 2019

@fitzgen I'm curious what times you get when running my perf test.

I'm not on the machine I was on earlier, but here is my linux laptop's timings:

- cargo metadata : 13.274993 ms
- rustc sysroot : 42.191570999999996 ms
- rustc version : 10.153625 ms
- wasm-bindgen version : 4.476614 ms
- wasm-pack version : 2.9216539999999998 ms

- cargo build  : 47.960446999999995 ms
- wasm-pack    : 132.383768 ms
- wasm-bindgen : 65.844551 ms

@Pauan
Copy link
Contributor

Pauan commented Jul 29, 2019

@fitzgen Thanks! rustc --version is a lot slower than wasm-bindgen --version, which is also what I see. It's interesting that rustc --print sysroot is so slow.

@alexcrichton
Copy link
Contributor

@Pauan hm do you have experience profiling applications on Windows? A 60ms performance overhead from executing rustup is pretty huge, that should be more along the the lines of 2-5ms at most. I'd be curious to learn more about what rustup is doing that's taking so long.

@Pauan also I'm curious, what version of Rust are you using? The stable version right now has a zillion dynamic libraries it needs to load where beta/nightly have a much smaller number of dynamic libraries to load, so I'm curious if that helps the performance at all.

@Pauan
Copy link
Contributor

Pauan commented Jul 31, 2019

@alexcrichton hm do you have experience profiling applications on Windows?

None at all. I had taken a look at some tools for Windows, but they tend to be proprietary, and none of them seem similar to Linux's perf.

what version of Rust are you using?

Latest stable, rustc 1.36.0 (a53f9df32 2019-07-03), though I get similar results with nightly rustc 1.38.0-nightly (dddb7fca0 2019-07-30):

- cargo metadata : 92.637771 ms
- rustc sysroot : 92.810626 ms
- rustc version : 83.76966399999999 ms
- rustc version (raw) : 19.204386 ms
- wasm-bindgen version : 17.115178 ms
- wasm-pack version : 19.787862 ms

- cargo build  : 183.779291 ms
- wasm-pack    : 594.6442 ms
- wasm-bindgen : 205.279656 ms

My Rustup also seems to be up to date: rustup 1.18.3 (435397f48 2019-05-22)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
current release current todo items feature request PR attached there's a PR open for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants