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

Don't pad .text to page size for pulley #10285

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

posborne
Copy link
Contributor

When targeting pulley we aren't directly emitting executable code in the .text section and we don't have a good idea of the true target page size so we end up with ELF files that can have a significant amount of extra padding around the .text section with no benefit to the consumer.

The change here aligns with the already present section flag SH_WASMTIME_NOT_EXECUTED. Padding elimination for the .rodata.wasm section is already handled by the presence/absence of the memory-init-on-cow configuration.

For a basic wasip1 hello-world rust program with the combination of this patch and -O memory-init-cow=n I saw a reduction in the compiled wasm ELF from 421KB to 189KB with the sections no longer showing as being padded out significantly:

$ objdump --section-headers target/wasm32-wasip1/release/hello-wasm-world-0x00.cwasm

target/wasm32-wasip1/release/hello-wasm-world-0x00.cwasm:       file format elf64-littleriscv

Sections:
Idx Name              Size     VMA              Type
  0                   00000000 0000000000000000
  1 .wasmtime.engine  00000353 0000000000000000 DATA
  2 .wasmtime.bti     00000001 0000000000000000 DATA
  3 .text             00011bdc 0000000000000000
  4 .wasmtime.addrmap 00011c5c 0000000000000000 DATA
  5 .wasmtime.traps   00000ae0 0000000000000000 DATA
  6 .rodata.wasm      000019e8 0000000000000000 DATA
  7 .name.wasm        000027a6 0000000000000000 DATA
  8 .wasmtime.info    000010f9 0000000000000000 DATA
  9 .symtab           00001788 0000000000000000
 10 .strtab           000040f0 0000000000000000
 11 .shstrtab         00000089 0000000000000000

Addresses #10244, CC @ia0 @tschneidereit


Feedback very welcome as this is a bit of a starter issue for me within wasmtime. Things I considered but didn't end up going with with this patch:

  • Directly inlining the conditional logic for the text align in the two callsites.
  • Using the SH_WASMTIME_NOT_EXECUTED instead of looking at the triple again for the alignment decision.
  • Possibly looking to see if it made sense to reuse memory-init-on-cow or some other flagging for this decision rather than using the triple.

I'm also open to any feedback regarding if there is any additional testing that should be added in tree for this patch

When targeting pulley we aren't directly emitting executable
code in the .text section and we don't have a good idea of the
true target page size so we end up with ELF files that can
have a significant amount of extra padding around the .text
section with no benefit to the consumer.

The change here aligns with the already present section flag
SH_WASMTIME_NOT_EXECUTED.  Padding elimination for the .rodata.wasm
section is already handled by the presence/absence of the
memory-init-on-cow configuration.

For a basic wasip1 hello-world rust program with the combination
of this patch and `-O memory-init-cow=n` I saw a reduction in
the compiled wasm ELF from 421KB to 189KB with the sections no
longer showing as being padded out significantly:

```
$ objdump --section-headers target/wasm32-wasip1/release/hello-wasm-world-0x00.cwasm

target/wasm32-wasip1/release/hello-wasm-world-0x00.cwasm:       file format elf64-littleriscv

Sections:
Idx Name              Size     VMA              Type
  0                   00000000 0000000000000000
  1 .wasmtime.engine  00000353 0000000000000000 DATA
  2 .wasmtime.bti     00000001 0000000000000000 DATA
  3 .text             00011bdc 0000000000000000
  4 .wasmtime.addrmap 00011c5c 0000000000000000 DATA
  5 .wasmtime.traps   00000ae0 0000000000000000 DATA
  6 .rodata.wasm      000019e8 0000000000000000 DATA
  7 .name.wasm        000027a6 0000000000000000 DATA
  8 .wasmtime.info    000010f9 0000000000000000 DATA
  9 .symtab           00001788 0000000000000000
 10 .strtab           000040f0 0000000000000000
 11 .shstrtab         00000089 0000000000000000
```
@posborne posborne requested a review from a team as a code owner February 24, 2025 21:16
@posborne posborne requested review from pchickey and removed request for a team February 24, 2025 21:16
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

This all looks great to me, thanks for working on this!

Using objdump --section-headers also makes me think that we should probably also provide a Config::* option for dropping the .symtab, .strtab, and .shstrtab sections. I don't think we have anything relying on that other than various debugging/perf utilities meaning it should be possible to strip them and still have a functioning *.cwasm binary.

I'll file an issue about this...

@alexcrichton alexcrichton added this pull request to the merge queue Feb 24, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 24, 2025
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.

2 participants