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

Investigate way to remove stack trace data from wasm #1570

Closed
ethanfrey opened this issue Jan 9, 2023 · 17 comments
Closed

Investigate way to remove stack trace data from wasm #1570

ethanfrey opened this issue Jan 9, 2023 · 17 comments

Comments

@ethanfrey
Copy link
Member

With the abort feature https://github.com/CosmWasm/cosmwasm we can now get useful error message when the contract panics. However, this also adds a number of strings with file paths and such into the wasm binary, which increases its size.

As some teams are working to trim down to get below the 800kB limit, and I saw such strings in their decoded wasm file, I suggested removing the abort feature to make it a bit smaller. They claimed to have done this and that is had no impact on the field size.

I assume this is due to the fact that the abort feature was still enabled via another dependency. It would be good to ensure this can be removed if desired. And also to document how to trim down contract size a bit.

@dakom Maybe you can attach some wasm files you got with and without the abort flag, so we can check them

@webmaster128
Copy link
Member

I assume this is due to the fact that the abort feature was still enabled via another dependency.

Yes, we need to ensure all libraries using cosmwasm-std disables default features. Otherwise contract developers have no chance to disable it.

@webmaster128
Copy link
Member

It would be very interesting to see the Wasm and the strings. It should only be relative file names and line numbers, i.e. not much data per panic. But I don't know how many of those panics we have in a typical contract.

@dakom
Copy link
Contributor

dakom commented Jan 9, 2023

here ya go

  • market-with-abort.wasm: cosmwasm-std = { version = "1.1.9", features = ["abort"] }
  • market-no-abort.wasm: cosmwasm-std = { version = "1.1.9"}
  • market-no-abort-or-default-features.wasm: cosmwasm-std = { version = "1.1.9", default-features = false}

that said, I only edited it in the contract's Cargo.toml, not any of its dependencies

contracts.zip

@dakom
Copy link
Contributor

dakom commented Jan 9, 2023

oh I should mention - these are built directly with cargo and wasm-opt, not with the docker tool

rustc 1.65.0 (897e37553 2022-11-02)
wasm-opt version 110 (version_110)

@webmaster128
Copy link
Member

okay, they are all the same:

sha256sum *
341fb44618f03518eedfffeb8adda13c373916d2542653615836722603862bdc  market-no-abort-or-default-features.wasm
341fb44618f03518eedfffeb8adda13c373916d2542653615836722603862bdc  market-no-abort.wasm
341fb44618f03518eedfffeb8adda13c373916d2542653615836722603862bdc  market-with-abort.wasm

No surprise given that at least cw-utils enables the feature by default. Others might to the same.

@dakom
Copy link
Contributor

dakom commented Jan 9, 2023

fwiw I found only 29 occurrences (thanks stackoverflow) and most of those by far are in dependencies, not the actual contract code (i.e. most of them are from some .cargo/registry/src/github.com/... path)

I do think it should be controllable, it's just not a major source of our particular binary size woes atm :)

@webmaster128
Copy link
Member

webmaster128 commented Jan 9, 2023

Yeah I see a lot of file paths and lower level panic messages.

But if you check wasm-objdump --headers market-no-abort.wasm you see that the code section is 14x as big as the entire data section. The data section is only responsible for 6.6% of the wasm file:

$ wasm-objdump --headers market-no-abort.wasm 

market-no-abort.wasm:	file format wasm 0x1

Sections:

     Type start=0x0000000b end=0x00000188 (size=0x0000017d) count: 49
   Import start=0x0000018b end=0x000002a5 (size=0x0000011a) count: 15
 Function start=0x000002a8 end=0x0000083b (size=0x00000593) count: 1425
    Table start=0x0000083d end=0x00000844 (size=0x00000007) count: 1
   Memory start=0x00000846 end=0x00000849 (size=0x00000003) count: 1
   Global start=0x0000084b end=0x00000864 (size=0x00000019) count: 3
   Export start=0x00000867 end=0x00000900 (size=0x00000099) count: 11
     Elem start=0x00000903 end=0x00000cf6 (size=0x000003f3) count: 1
     Code start=0x00000cfa end=0x000b9ec7 (size=0x000b91cd) count: 1425
     Data start=0x000b9ecb end=0x000c70c3 (size=0x0000d1f8) count: 38

So even if you cut string literals in half, we are talking about a few percentage points. I think we need to look for unnecessary code.

@webmaster128
Copy link
Member

What's concerning me more is that machine dependent paths are exposed (including your username and indirectly your OS). This highlights that Wasm blobs should not be generated without a containerized builder.

@webmaster128
Copy link
Member

Those 1400+ functions is something suspecious. I did not find other contracts with more than 500 functions. Maybe this is due to features but maybe this is the result of a certain development style.

Do you use a lot of generics at high level? Just a random idea how to create many implementations for the same functionality.

@dakom
Copy link
Contributor

dakom commented Jan 9, 2023

nah, not a crazy amount of generics... and even where there are some generics, it's mostly over a couple number types like u32 and u64, so not a whole lot of monomorphization going on. but it is fairly monolithic, lots of business logic is in one contract (to save on gas fees with smart queries, basically)

btw here's a build w/ docker tool
market.wasm.zip

@ethanfrey
Copy link
Member Author

Those 1400+ functions is something suspecious. I did not find other contracts with more than 500 functions. Maybe this is due to features but maybe this is the result of a certain development style.

This is very likely serde. I came across all kinds of generics from that when debugging wasm builds for floats in the past.

I think replacing the JSON lib would be a great way to shrink the code.
Also a great way to make developer ergonomics much worse.

It would be worth a spike to see what would happen to gas and wasm size if we were to use eg miniserde on a non-trivial contract. I mean, it would need a fork of cosmwasm-std as well... and probably have it's own float issues....

Nah, not really a path to walk on unless you just want to show "what if"

@dakom
Copy link
Contributor

dakom commented Jan 10, 2023

lmk if y'all think it would be helpful to look at our source. I don't have the authority to approve directly, but we can discuss off-line and hopefully find a way to grant early access (ofc we eventually plan to open source, it's just closed for now since we're still in active development/innovate mode)

@webmaster128
Copy link
Member

I doubt there is much more we can do than highlighting that by looking at the Wasm blob you see 93% is of the file size is code, which does not include the string literals. Building large monolithic contracts is possible but the chain has to open up the size limit and you need to test resource consumption when doing so (memory, CPU, module size on disk ...). It is well possible that this is much better than splitting the contract.

@mikedotexe
Copy link

oh I should mention - these are built directly with cargo and wasm-opt, not with the docker tool

This may or may not be helpful, but when I was at NEAR we hired the guy who wrote the Rust Analyzer (https://github.com/matklad) and did a deep dive into contract optimization and found there to be a deep issue showing paths. So we also decided to use Docker so we could have deterministic binaries.

Here's also a set of advice I helped write that covers some low-hanging fruit like not using .expect and assert(s) and other calls which will generate a stack trace:
https://docs.near.org/sdk/rust/contract-size#small-wins

(Not all of these apply, but could be helpful)

@webmaster128
Copy link
Member

In #1595 I created documentation for how to use cosmwasm-std as a library. In there I strongly recommend disabling all default features. If libraries follow the advise it becomes possible to sidable the abort and iterator feature in the contract.

cw-utils does currenly not comply. Not sure about other libraries using cosmwasm-std.

@webmaster128
Copy link
Member

Thanks for the pointer @mikedotexe. One thing we can easily provide is a version of env::panic_str called cosmwasm_std::abort which immediately terminates the contract with the given message. In contrast to the panic handler and the panic! macro, this would avoid adding file paths and line numbers to the panic message. However, I'm not sure if this is good for unit testing as we suddenly need a way to handle those abortions in unit tests.

@chipshort
Copy link
Collaborator

Closing because we cannot avoid the fact that the Wasm contains the file positions (as mentioned here).

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

5 participants