Skip to content

Remove some dependencies #2730

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

Merged
merged 4 commits into from
May 4, 2021
Merged

Conversation

bjorn3
Copy link
Contributor

@bjorn3 bjorn3 commented Mar 16, 2021

  • Remove the thiserror dependency from cranelift-codegen and cranelift-module and use a cleaned up version of it's derives expansion instead.
    This improves compilation time of cg_clif by 6s. For users of cranelift that don't already disable optimizations for syn and other dependencies of the thiserror proc macro, the release mode compilation time wins will be even larger.
  • Remove some unnecessary dependencies as found by cargo-udeps.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:module labels Mar 16, 2021
@pchickey
Copy link
Contributor

I approve these changes in cranelift-module. I'm not a maintainer of cranelift-codegen so I will defer overall approval to someone else - @cfallin maybe?

@cfallin
Copy link
Member

cfallin commented Mar 18, 2021

It's somewhat surprising to me that changes in error-printing code would give such a large speedup in compile times. It's always welcome, of course, when speedups come -- but @bjorn3, do you have any sense of why this happens? I ask because it could be a symptom of something weirder going on behind the scenes -- for example, are we throwing a bunch of errors and doing work to capture backtraces but then silencing them somehow?

All other things being equal, the auto-derived error handling is kind of nice, but I won't fight too hard for it if we win by doing something lighter-weight.

@pchickey
Copy link
Contributor

pchickey commented Mar 18, 2021

I believe, because thiserror is a proc macro that depends on the usual stack of syn and friends, that compiling those dependencies is the tentpole in embeddings which otherwise don't use proc macros. So, we don't see this in wasmtime or other big applications where we use that family of crates in several contexts, but I imagine that cg_clif is very careful about deps.

@cfallin
Copy link
Member

cfallin commented Mar 18, 2021

Ah -- to clarify, @bjorn3, you mean the compile time of cg_clif, not the time that cg_clif takes to compile code?

(I was assuming the latter, and thus was quite confused about the impact of error-path code on the happy path!)

@bjorn3
Copy link
Contributor Author

bjorn3 commented Mar 18, 2021

Ah -- to clarify, @bjorn3, you mean the compile time of cg_clif, not the time that cg_clif takes to compile code?

Correct, I mean the time it takes to compile cg_clif itself. Sorry for the confusion.

but I imagine that cg_clif is very careful about deps.

Indeed. I try to avoid dependencies where possible and even use dependencies from the rustc-dev rustup component instead of crates.io where possible.

Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

Looks good -- I definitely am in agreement with the idea that our core libraries should try not to pull too much in. (IIRC this has been stated as a goal at one time or another but the tendency is always to drift in the other direction...) Happy to merge once the conflicts are resolved.

@pchickey
Copy link
Contributor

Would you mind leaving a comment above the Display & Error impls that these are written by hand to avoid pulling in the thiserror dep? That may help anyone who sees them and gets the clever idea that they can be replaced by a macro.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Mar 19, 2021

Done

@bjorn3 bjorn3 force-pushed the less_thiserror branch from b696f30 to 82f3ad4 Compare May 4, 2021 11:51
@bjorn3
Copy link
Contributor Author

bjorn3 commented May 4, 2021

Rebased

@tschneidereit tschneidereit requested a review from cfallin May 4, 2021 13:06
@pchickey pchickey merged commit fe76c59 into bytecodealliance:main May 4, 2021
@bjorn3 bjorn3 deleted the less_thiserror branch May 4, 2021 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:module cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants