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

Reduce the number of dependencies #139

Open
jonhoo opened this issue Jul 24, 2019 · 6 comments
Open

Reduce the number of dependencies #139

jonhoo opened this issue Jul 24, 2019 · 6 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@jonhoo
Copy link
Owner

jonhoo commented Jul 24, 2019

$ cargo tree --no-indent | sed 's/ (\*)//' | sort -u | wc -l
126

My guess is that some of these are unnecessary. Let's see if we can't do something to help improve build times.

@jonhoo jonhoo added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Jul 24, 2019
@felippemr
Copy link

Hey @jonhoo I tried building the project locally without each one of the non dev deps but the project would only build with all of them. What is the path you were thinking for this?

@felippemr
Copy link

When I try to cargo build the project locally I get:

 --> src/collapse/perf.rs:4:5
  |
4 | use symbolic_demangle::demangle;
  |     ^^^^^^^^^^^^^^^^^ use of undeclared type or module `symbolic_demangle`

error[E0433]: failed to resolve: use of undeclared type or module `symbolic_demangle`
   --> src/collapse/dtrace.rs:332:45
    |
332 |         self.transform_function_name(frame, symbolic_demangle::demangle)
    |                                             ^^^^^^^^^^^^^^^^^ use of undeclared type or module `symbolic_demangle`

error: aborting due to 2 previous errors

@jonhoo
Copy link
Owner Author

jonhoo commented Aug 20, 2019

Ah, sorry, to be clear, it's not as though we currently list unused dependencies, but rather I would like to see us re-write parts of the code so that we can remove dependencies. I'm not sure what the issue you are pointing to with symbolic_demangle is -- it compiles fine for me locally?

@jasonrhansen
Copy link
Collaborator

I think we should be able to remove symbolic_demangle as a dependency.

#133 and #134 added to our collapsers the ability to fix partially demangled Rust symbols with no external dependencies. I originally implemented #132 to work around the same problem, but it required you to pass --no-demangle to perf script, or -xmangled to dtrace since symbolic_demangle doesn't work when the symbols are partially demangled. Now that we are able to take the mostly demangled symbols the profiling tools give us and fix them up from there, the demangle option on the collapsers seems to be unnecessary.

What do you think?

@jasonrhansen
Copy link
Collaborator

Now we're down to 109 dependencies after #144.

$ cargo tree --no-indent | sed 's/ (\*)//' | sort -u | wc -l
109

@jasonrhansen
Copy link
Collaborator

#145, #146, and #147 have reduced dependencies further and made more of them optional.

$ cargo tree --no-indent | sed 's/ (\*)//' | sort -u | wc -l
107
$ cargo tree --no-indent --no-default-features | sed 's/ (\*)//' | sort -u | wc -l
76

@jonhoo jonhoo removed the good first issue Good for newcomers label Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants