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

More heap allocations than structopt #4956

Closed
dullbananas opened this issue Jun 8, 2023 · 6 comments
Closed

More heap allocations than structopt #4956

dullbananas opened this issue Jun 8, 2023 · 6 comments

Comments

@dullbananas
Copy link

I tested the heap usage of wasm-pack when using it to compile wasm_game_of_life before and after upgrading from structopt to clap 4.3. The total allocated bytes increased by 22%, and the maximum allocated bytes (t-gmax) increased by 57%.

dhat-heap.zip

@dullbananas
Copy link
Author

Dhat files can be viewed here https://nnethercote.github.io/dh_view/dh_view.html

@epage
Copy link
Member

epage commented Jun 8, 2023

Could you include a reproduction case?

@dullbananas
Copy link
Author

git clone https://github.com/dullbananas/wasm-pack.git
cd wasm-pack
# switch to version with structopt
git checkout dc9e99c
# clone inside of wasm-pack
git clone https://github.com/rustwasm/wasm_game_of_life.git v
cd v
git checkout fc35b7c
cd ..
# run twice the first time because the very first run of wasm-pack does more heap allocations
cargo run --release --features "dhat-heap" -- build --release v
cargo run --release --features "dhat-heap" -- build --release v
# switch to version with structopt
git checkout 0692377
# this will overwrite dhat-heap.json
cargo run --release --features "dhat-heap" -- build --release v

@epage
Copy link
Member

epage commented Jun 8, 2023

Also, could you describe the end-user impact. Is this noticeable in any way? For example, in #4774 we are discussing the impact of clap initialization time on the full startup time of the application, affecting the more interactive experience of the user.

@dullbananas
Copy link
Author

I found no measurable difference in initialization time. To measure it, I added this code after let args = Cli::parse():

dbg!(args); // prevent parsing from being optimized away
return Ok(());

And I ran this 3 times:

time ./target/*/release/wasm-pack build --release v

Structopt: 0.158s, 0.010s, 0.008s

Clap: 0.168s, 0.011s, 0.009s

I also measured the binary size.

Structopt: 12,029,348 bytes

Clap: 12,160,608 bytes (1.1% increase)

And I used dbg!(dhat::HeapStats::get().curr_bytes) to measure the heap usage after parsing is done.

Structopt: 1130 bytes

Clap: 978 bytes

@epage
Copy link
Member

epage commented Jun 9, 2023

Sounds like the end-user impact of these increased allocations is negligible which has me leaning towards closing this. If you have a reason we should actively focus on this, let us know!

I suspect some of our work towards #2037 and #1365 might help. We are exploring making more settings of Command and Arg runtime defined (e.g. #4833). This will decrease the size of Command and Arg though it means there will be extra allocations for any of these runtime fields used.

#4959 would be the biggest change for reducing allocations as it will make it so we don't even instantiate Args that are never used and we wouldn't be initializing some of the fields of unused Commands.

@epage epage closed this as not planned Won't fix, can't repro, duplicate, stale Jun 9, 2023
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

No branches or pull requests

2 participants