Add --all(-generics|-monos) flags to monos command #120

userzimmermann · 2018-08-13T17:05:41Z

After #118 the second PR for #109

This time for the monos command, and with even three new flags :D to cover the 2-dimensional output of generics and their individual monomorphizations:

-a, --all
    --all-generics
    --all-monos

Only some more tests are needed to get it out of WIP... I'll hurry up ;)

data-pup · 2018-08-13T17:58:43Z

Awesome! Feel free to ping me when you think this is ready for review. Love the idea of multiple flags!

userzimmermann · 2018-08-14T12:15:14Z

@data-pup: ping :)

I added the remaining tests... Unfortunately, in the new monos_all_monos test output for the --all-monos flag, there is no generic function with more individual monomorphizations than the default max_monos = 10. So the effect of --max-monos is not so visible, except that there are Σ [237 Total Rows] instead of the Σ [235 Total Rows] of the default monos test output. So I was thinking about adding --max-generics 11 to the test call to also get that core::ptr::drop_in_place generic listed, the only one with more than 10 monomorphizations: https://github.com/rustwasm/twiggy/pull/120/files#diff-b08b9997ee2e3dd09c6414a42623db04R51 But then I thought I'm thinking too much ;P Or what's your opinion about that?

And BTW: I also changed the existing --max-monos flag description a bit into ... for each listed generic ... instead of just ... for each generic ... To make it a bit clearer ;) https://github.com/rustwasm/twiggy/pull/120/files#diff-e6bf097a1c8cd4d9cfc57a634c427921R370

userzimmermann · 2018-08-14T12:21:01Z

@data-pup: The failing TravisCI build is not my fault ;P What actually happened there? https://travis-ci.org/rustwasm/twiggy/jobs/415881546#L544 Where does this sudden incompatibility come from? I'm very curious :D

userzimmermann · 2018-08-14T13:37:02Z

@data-pup: I tracked down the TravisCI issue... So this is now blocked by #122 :D

data-pup · 2018-08-14T14:07:47Z

Nice work! I should have some time to review this tonight 👍

userzimmermann · 2018-08-14T18:24:08Z

Oops! Can you please restart https://travis-ci.org/rustwasm/twiggy/jobs/415989407 ? :)

data-pup · 2018-08-15T02:22:38Z

This all looks good to me 😸 Definitely a fan of separate flags for displaying all generics and all monomorphizations, seems like a nice distinction to make. Regarding this point that you brought up:

So I was thinking about adding --max-generics 11 to the test call to also get that core::ptr::drop_in_place generic listed, the only one with more than 10 monomorphizations.

It might be nice to add a way to name specific generic functions that we would like to display, so that we could narrow down our test expectations a little. For now however, I think that's outside of the scope of this PR, and worth handling as a separate issue.

Because this does add a couple different flags, I'd like to wait for @fitzgen to check this out too before merging, but this looks good to me. Nice work!

fitzgen

LGTM with one nitpick below fixed :)

fitzgen · 2018-08-15T16:14:05Z

opt/definitions.rs

@@ -403,13 +422,15 @@ impl Monos {

 /// The maximum number of generics to list.
 pub fn max_generics(&self) -> u32 {
- self.max_generics
+ if self.all_generics_and_monos || self.all_generics { u32::MAX }
+ else { self.max_generics }


Let me guess: you've done a bunch of lisp/scheme in the past haven't you, @userzimmermann? :-p

Let's stick to the rustfmt style: run cargo fmt on the project and make a new commit for the restyling.

@fitzgen

Let me guess: you've done a bunch of lisp/scheme in the past haven't you

And all of that in Emacs ofc! But would've never thought that this would be visible here :D You already know myself better than I do ;P

@fitzgen: Hmm... cargo fmt only did:

diff --git a/opt/opt.rs b/opt/opt.rs index b1646da..6c0fc5c 100644 --- a/opt/opt.rs +++ b/opt/opt.rs @@ -1,10 +1,7 @@ //! Options for running `twiggy`. #![deny(missing_debug_implementations)] -#![cfg_attr( - feature = "wasm", - feature(use_extern_macros) -)] +#![cfg_attr(feature = "wasm", feature(use_extern_macros))] #[macro_use] extern crate cfg_if; diff --git a/twiggy/tests/tests.rs b/twiggy/tests/tests.rs index e3d8633..33302ce 100644 --- a/twiggy/tests/tests.rs +++ b/twiggy/tests/tests.rs @@ -455,7 +455,11 @@ test!( "2" ); -test!(garbage_wee_alloc_top_10, "garbage", "./fixtures/wee_alloc.wasm"); +test!( + garbage_wee_alloc_top_10, + "garbage", + "./fixtures/wee_alloc.wasm" +); test!( garbage_wee_alloc_all,

Which is both not related to this PR :/ So somehow my lispy/schemey style seems to be fine ;)

Can cargo fmt also be used for linting btw? Would be nice to see such style issues automatically in PRs... I could implement some RustFmtBear in https://github.com/coala/coala-bears and then we could add https://gitmate.io as additional CI service for doing automated code analysis... Besides other nice stuff ;)

Ah! cargo fmt --all -- --check can be used for linting :) According to https://github.com/rust-lang-nursery/rustfmt#checking-style-on-a-ci-server

clippy can also help with linting, if you're looking to check style details while you're working 👍

@data-pup @fitzgen: Running rustfmt --check definitions.rs indeed reveals a lot :D

Also that I didn't stick to the rules in #118 ;P

Diff in \\?\C:\Users\userz\Projects\rust\wasm\twiggy\opt\definitions.rs at line 1: - // Fun times ahead! // // Apparently, proc-macros don't play well with `cfg_attr` yet, and their Diff in \\?\C:\Users\userz\Projects\rust\wasm\twiggy\opt\definitions.rs at line 9: // It's terrible! But it works for now. /// Options for configuring `twiggy`. -#[derive(Clone, Debug)] -#[derive(StructOpt)] -#[structopt(about = "\n`twiggy` is a code size profiler.\n\nIt analyzes a binary's call graph to answer questions like:\n\n* Why was this function included in the binary in the first place?\n\n* What is the retained size of this function? I.e. how much space\n would be saved if I removed it and all the functions that become\n dead code after its removal.\n\nUse `twiggy` to make your binaries slim!")] +#[derive(Clone, Debug, StructOpt)] +#[structopt( + about = "\n`twiggy` is a code size profiler.\n\nIt analyzes a binary's call graph to answer questions like:\n\n* Why was this function included in the binary in the first place?\n\n* What is the retained size of this function? I.e. how much space\n would be saved if I removed it and all the functions that become\n dead code after its removal.\n\nUse `twiggy` to make your binaries slim!" +)] pub enum Options { /// List the top code size offenders in a binary. #[structopt(name = "top")] Diff in \\?\C:\Users\userz\Projects\rust\wasm\twiggy\opt\definitions.rs at line 39: /// Find and display code and data that is not transitively referenced by /// any exports or public functions. #[structopt(name = "garbage")] - Garbage(Garbage) + Garbage(Garbage), } /// List the top code size offenders in a binary. Diff in \\?\C:\Users\userz\Projects\rust\wasm\twiggy\opt\definitions.rs at line 46: -#[derive(Clone, Debug)] -#[derive(StructOpt)] +#[derive(Clone, Debug, StructOpt)] #[wasm_bindgen] pub struct Top { /// The path to the input binary to size profile. Diff in \\?\C:\Users\userz\Projects\rust\wasm\twiggy\opt\definitions.rs at line 130: } /// Compute and display the dominator tree for a binary's call graph. -#[derive(Clone, Debug, Default)] -#[derive(StructOpt)] +#[derive(Clone, Debug, Default, StructOpt)] #[wasm_bindgen] pub struct Dominators { /// The path to the input binary to size profile. Diff in \\?\C:\Users\userz\Projects\rust\wasm\twiggy\opt\definitions.rs at line 214: /// Find and display the call paths to a function in the given binary's call /// graph. -#[derive(Clone, Debug)] -#[derive(StructOpt)] +#[derive(Clone, Debug, StructOpt)] #[wasm_bindgen] pub struct Paths { /// The path to the input binary to size profile. Diff in \\?\C:\Users\userz\Projects\rust\wasm\twiggy\opt\definitions.rs at line 336: /// List the generic function monomorphizations that are contributing to /// code bloat. -#[derive(Clone, Debug)] -#[derive(StructOpt)] +#[derive(Clone, Debug, StructOpt)] #[wasm_bindgen] pub struct Monos { /// The path to the input binary to size profile. Diff in \\?\C:\Users\userz\Projects\rust\wasm\twiggy\opt\definitions.rs at line 418: /// The maximum number of generics to list. pub fn max_generics(&self) -> u32 { - if self.all_generics_and_monos || self.all_generics { u32::MAX } - else { self.max_generics } + if self.all_generics_and_monos || self.all_generics { + u32::MAX + } else { + self.max_generics + } } /// The maximum number of individual monomorphizations to list for each Diff in \\?\C:\Users\userz\Projects\rust\wasm\twiggy\opt\definitions.rs at line 429: /// generic function. pub fn max_monos(&self) -> u32 { - if self.all_generics_and_monos || self.all_monos { u32::MAX } - else { self.max_monos } + if self.all_generics_and_monos || self.all_monos { + u32::MAX + } else { + self.max_monos + } } /// Set whether to hide individual monomorphizations and only show the Diff in \\?\C:\Users\userz\Projects\rust\wasm\twiggy\opt\definitions.rs at line 464: } /// Diff the old and new versions of a binary to see what sizes changed. -#[derive(Clone, Debug)] -#[derive(StructOpt)] +#[derive(Clone, Debug, StructOpt)] #[wasm_bindgen] pub struct Diff { /// The path to the old version of the input binary. Diff in \\?\C:\Users\userz\Projects\rust\wasm\twiggy\opt\definitions.rs at line 524: /// Find and display code and data that is not transitively referenced by any /// exports or public functions. -#[derive(Clone, Debug)] -#[derive(StructOpt)] +#[derive(Clone, Debug, StructOpt)] #[wasm_bindgen] pub struct Garbage { /// The path to the input binary to size profile. Diff in \\?\C:\Users\userz\Projects\rust\wasm\twiggy\opt\definitions.rs at line 576: /// The maximum number of items to display. pub fn max_items(&self) -> u32 { - if self.all_items { u32::MAX } else { self.max_items } + if self.all_items { + u32::MAX + } else { + self.max_items + } } /// Set the maximum number of items to display.

Awesome! I think the final step now is to take care of those if/else statements' formatting, and this can be merged 👍 The #[derive(..)] attributes can probably be left alone, I think they potentially play into how the build.rs file works

@data-pup: Actually build.rs only removes either #[wasm_bindgen] or #[structopt(...)] lines depending on the target:

twiggy/opt/build.rs

Lines 20 to 21 in 26de56d

copy_without_lines_matching_pattern("definitions.rs", cli, ".*\\bwasm_bindgen\\b.*");

copy_without_lines_matching_pattern("definitions.rs", wasm, ".*\\bstructopt\\b.*");

So merging the #[derive(...)] attributes would be actually fine... But ofc not as part of this PR ;) I will put that in another one :D And by simplifying build.rs a bit to not process definitions.rs line-wise, even the #[structopt(...)]s could be multi-lined... But that's ofc not even part of that other proposed PR ;P

And now I need to find a better internet connection...

Ah, good eye! Hadn't delved into that file too much quite yet, good to know 😸 Feel free to ping me once the formatting changes are made and I can merge this. Looking forward to any other PR's too! 🎉

@data-pup

Ah, good eye!

Not good enough ;) I didn't see the .case_insensitive(true) for RegexBuilder... So the #[derive(StructOpt)] lines also get removed for writing wasm.rs... But anyway! All that can be optimized in build.rs... for sure... later :) To reach full rustfmt comformity in definitions.rs :D

For now I made the 2 if/else format changes and re-pushed

With an additional -a shortcut for the simple --all flag that combines listing all generics and monomorphizations Also extends tests accordingly Relates to rustwasm#109

data-pup · 2018-08-21T22:34:47Z

Awesome, thanks @userzimmermann! 🎉

data-pup mentioned this pull request Aug 13, 2018

Add -a, --all flag to garbage command #118

Merged

userzimmermann force-pushed the zimmermann/all-monos branch from d8fada9 to 475bc73 Compare August 14, 2018 11:44

userzimmermann changed the title ~~WIP: Add --all(-generics|-monos) flags to monos command~~ Add --all(-generics|-monos) flags to monos command Aug 14, 2018

data-pup self-requested a review August 14, 2018 14:07

userzimmermann force-pushed the zimmermann/all-monos branch from 475bc73 to 50711aa Compare August 14, 2018 16:08

userzimmermann mentioned this pull request Aug 14, 2018

Remove stabilized wasm feature attributes #122

Merged

data-pup approved these changes Aug 15, 2018

View reviewed changes

fitzgen approved these changes Aug 15, 2018

View reviewed changes

data-pup mentioned this pull request Aug 15, 2018

Add arguments to monos command #125

Closed

Add --all(-generics|-monos) flags to monos command

fbc5090

With an additional -a shortcut for the simple --all flag that combines listing all generics and monomorphizations Also extends tests accordingly Relates to rustwasm#109

userzimmermann force-pushed the zimmermann/all-monos branch from 50711aa to fbc5090 Compare August 20, 2018 19:42

data-pup merged commit f745992 into rustwasm:master Aug 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --all(-generics|-monos) flags to monos command #120

Add --all(-generics|-monos) flags to monos command #120

userzimmermann commented Aug 13, 2018

data-pup commented Aug 13, 2018

userzimmermann commented Aug 14, 2018 •

edited

Loading

userzimmermann commented Aug 14, 2018

userzimmermann commented Aug 14, 2018

data-pup commented Aug 14, 2018

userzimmermann commented Aug 14, 2018

data-pup commented Aug 15, 2018 •

edited

Loading

fitzgen left a comment

fitzgen Aug 15, 2018

userzimmermann Aug 17, 2018

userzimmermann Aug 17, 2018 •

edited

Loading

userzimmermann Aug 17, 2018 •

edited

Loading

data-pup Aug 19, 2018

userzimmermann Aug 20, 2018

data-pup Aug 20, 2018 •

edited

Loading

userzimmermann Aug 20, 2018 •

edited

Loading

data-pup Aug 20, 2018

userzimmermann Aug 20, 2018

data-pup commented Aug 21, 2018

	copy_without_lines_matching_pattern("definitions.rs", cli, ".\\bwasm_bindgen\\b.");
	copy_without_lines_matching_pattern("definitions.rs", wasm, ".\\bstructopt\\b.");

Add --all(-generics|-monos) flags to monos command #120

Add --all(-generics|-monos) flags to monos command #120

Conversation

userzimmermann commented Aug 13, 2018

data-pup commented Aug 13, 2018

userzimmermann commented Aug 14, 2018 • edited Loading

userzimmermann commented Aug 14, 2018

userzimmermann commented Aug 14, 2018

data-pup commented Aug 14, 2018

userzimmermann commented Aug 14, 2018

data-pup commented Aug 15, 2018 • edited Loading

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen Aug 15, 2018

Choose a reason for hiding this comment

userzimmermann Aug 17, 2018

Choose a reason for hiding this comment

userzimmermann Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

userzimmermann Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

data-pup Aug 19, 2018

Choose a reason for hiding this comment

userzimmermann Aug 20, 2018

Choose a reason for hiding this comment

data-pup Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

userzimmermann Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

data-pup Aug 20, 2018

Choose a reason for hiding this comment

userzimmermann Aug 20, 2018

Choose a reason for hiding this comment

data-pup commented Aug 21, 2018

userzimmermann commented Aug 14, 2018 •

edited

Loading

data-pup commented Aug 15, 2018 •

edited

Loading

userzimmermann Aug 17, 2018 •

edited

Loading

userzimmermann Aug 17, 2018 •

edited

Loading

data-pup Aug 20, 2018 •

edited

Loading

userzimmermann Aug 20, 2018 •

edited

Loading