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

Adding "optimization-passes" option #216

Merged
merged 18 commits into from
Mar 18, 2021

Conversation

trace-andreason
Copy link
Contributor

Addressing issue #169

src/cmd/build.rs Outdated
@@ -66,6 +66,8 @@ pub struct BuildCommand {
verbosity: VerbosityFlags,
#[structopt(flatten)]
unstable_options: UnstableOptions,
#[structopt(long = "optimization-passes", default_value = "3")]
optimization_passes: u32,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to see this restricted to the valid values, perhaps by introducing an enum type. Also if you look at wasm-opt --help there are a couple of non integer values possible:

 -O0                                          execute no optimization passes 
   -O1                                          execute -O1 optimization passes
                                                (quick&useful opts, useful for 
                                                iteration builds) 
   -O2                                          execute -O2 optimization passes
                                                (most opts, generally gets most
                                                perf) 
   -O3                                          execute -O3 optimization passes
                                                (spends potentially a lot of 
                                                time optimizing) 
   -O4                                          execute -O4 optimization passes
                                                (also flatten the IR, which can
                                                take a lot more time and memory,
                                                but is useful on more nested / 
                                                complex / less-optimized input)
   -Os                                          execute default optimization 
                                                passes, focusing on code size 
   -Oz                                          execute default optimization 
                                                passes, super-focusing on code 
                                                size 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ascjones ahhh, I had assumed it was limited to integers as it was typed that way in the do_optimization function. I'll give the enum a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, if we wanted to go the enum route, it might create some problems with the binaryen-as-dependency optionsfor installation which seems to only take a u32 for optimization passes: https://docs.rs/binaryen/0.12.0/src/binaryen/lib.rs.html#21-28

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can always convert from the enum to the u32 at the call site. Would be worth finding out if 5 -> s and 6 -> z. At the very least we should validate the input value so it can't just be any u32.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

did a little more digging on the binaryen crate:

/// Codegen configuration.
#[derive(Default)]
pub struct CodegenConfig {
    /// 0, 1, 2 correspond to -O0, -Os, -Oz
    pub shrink_level: u32,
    /// 0, 1, 2 correspond to -O0, -O1, -O2, etc.
    pub optimization_level: u32,
    /// If set, the names section is emitted.
    pub debug_info: bool,
}

I made the shrink options default to 3 optimization passes in the case of binaryen as a dependency

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome, I'd like @cmichi to take a look at this too since he's more familiar with the wasm-opt integration

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right now this is the cli help output:

$ cargo run -- contract build --help
…
        --optimization-passes <optimization-passes>    
             [default: 3]

What I would like to see is either

  • a note with possible params and a sentence noting that they are simply forwarded to wasm-opt or
  • a list of all possible values with each ones description (like the list which @ascjones posted above).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cmichi added. Here is what the new cli help output looks like:

        --optimization-passes <optimization-passes>
            number of optimization passes, passed as an argument to wasm-opt
            
            - `0`: execute no optimization passes
            
            - `1`: execute 1 optimization pass (quick & useful opts, useful for iteration builds)
            
            - `2`, execute 2 optimization passes (most opts, generally gets most perf)
            
            - `3`, execute 3 optimization passes (spends potentially a lot of time optimizing)
            
            - `4`, execute 4 optimization passes (also flatten the IR, which can take a lot more time and memory but is
            useful on more nested / complex / less-optimized input)
            
            - `s`, execute default optimization passes, focusing on code size
            
            - `z`, execute default optimization passes, super-focusing on code size
            
            - [default: 3]

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
trace-andreason and others added 7 commits March 11, 2021 09:33
Co-authored-by: Michael Müller <mich@elmueller.net>
Co-authored-by: Michael Müller <mich@elmueller.net>
Co-authored-by: Michael Müller <mich@elmueller.net>
Co-authored-by: Michael Müller <mich@elmueller.net>
Co-authored-by: Michael Müller <mich@elmueller.net>
src/cmd/build.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@cmichi cmichi left a comment

Choose a reason for hiding this comment

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

Thanks @trace-andreason !

@cmichi cmichi merged commit afc190b into use-ink:master Mar 18, 2021
@cmichi cmichi mentioned this pull request Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants