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

Remove rayon dependency of cranelift-isle #5101

Merged
merged 1 commit into from
Oct 23, 2022

Conversation

bjorn3
Copy link
Contributor

@bjorn3 bjorn3 commented Oct 23, 2022

Using rayon adds a lot of dependencies to Cranelift. The total unparallelized time the code that uses rayon takes is less than half a second and it runs at compile time, so there is pretty much no benefit to parallelizing it.

See https://github.com/bytecodealliance/wasmtime/pull/4906/files#r1002697557. I think I'm going to revert back to Cranelift 0.88.1 until this is merged.

Using rayon adds a lot of dependencies to Cranelift. The total
unparallelized time the code that uses rayon takes is less than half a
second and it runs at compile time, so there is pretty much no benefit
to parallelizing it.
@bjorn3
Copy link
Contributor Author

bjorn3 commented Oct 23, 2022

For reference the extra dependencies increase build time of cg_clif with unstable features (jit and inline asm) disabled from 1min 35s to 2min 5s. Or in other words a 30% increase.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language labels Oct 23, 2022
@github-actions
Copy link

Subscribe to Label Action

cc @cfallin, @fitzgen

This issue or pull request has been labeled: "cranelift", "isle"

Thus the following users have been cc'd because of the following labels:

  • cfallin: isle
  • fitzgen: isle

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

bjorn3 added a commit to rust-lang/rustc_codegen_cranelift that referenced this pull request Oct 23, 2022
It added a lot of extra dependencies. I opened bytecodealliance/wasmtime#5101
to remove those dependencies again.

This reverts commit da770ab.
@bjorn3
Copy link
Contributor Author

bjorn3 commented Oct 23, 2022

Reverted cg_clif's dependency on Cranelift to 0.88.1 in bjorn3/rustc_codegen_cranelift@266e967.

Copy link
Member

@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me if it's not increasing isle compilation time substantially.

@elliottt elliottt merged commit 470070a into bytecodealliance:main Oct 23, 2022
@bjorn3 bjorn3 deleted the no_isle_rayon branch October 23, 2022 22:50
alexcrichton pushed a commit to alexcrichton/wasmtime that referenced this pull request Oct 26, 2022
Using rayon adds a lot of dependencies to Cranelift. The total
unparallelized time the code that uses rayon takes is less than half a
second and it runs at compile time, so there is pretty much no benefit
to parallelizing it.
alexcrichton added a commit that referenced this pull request Oct 26, 2022
* Fix push tag workflow (#5082)

This commit fixes the `push-tag.yml` workflow to work with the new
`Cargo.toml` manifest since workspace inheritance was added. This
additionally fixes some warnings coming up on CI about our usage of
deprecated features on github actions.

* Reduce warnings on CI from GitHub Actions (#5083)

* Upgrade our github actions to "node16"

Each github actions run has a lot of warnings about using node12 so this
upgrades our repository to using node16. I'm hoping no other changes are
needed and I suspect other actions we're using are on node12 and will
need further updates, but this should help pin down what's remaining.

* Update `actions/checkout` workflow to `v3`

* Update to `actions/cache@v3`

* Update to `actions/upload-artifact@v3`

* Drop usage of `actions-rs/toolchain`

* Update to `actions/setup-python@v4`

* Update mdbook version

* Add `package-lock.json` for `github-release` action (#5091)

A local github action we have has been broken for about a month now
meaning that the `dev` tag isn't getting updated or getting new
releases. This appears to be due to the publication of new versions of
these dependencies which are running into issues using one another. I
think I've figured out versions that work and have added a
`package-lock.json` to ensure we keep using the same versions.

* More fixes for publish action (#5110)

Looks like #5091 wasn't enough and some of the APIs needed updating with
changes made in the meantime. I've updated the action here and
additionally made a separate change where the release isn't continually
created and deleted but instead left alone and only the tag is updated.
This should work for the `dev` release and avoids deleting/recreating on
each PR, sending out notifications for new releases.

* Add missing `Win32_Foundation` feature

This is necessary for the `wasmtime-runtime` crate to compile on Windows.

* Add a note for the 2.0.1 release

* Remove rayon dependency of cranelift-isle (#5101)

Using rayon adds a lot of dependencies to Cranelift. The total
unparallelized time the code that uses rayon takes is less than half a
second and it runs at compile time, so there is pretty much no benefit
to parallelizing it.

* Add a note about rayon removal

Co-authored-by: Christopher Serr <christopher.serr@gmail.com>
Co-authored-by: bjorn3 <17426603+bjorn3@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants