Don't pad .text to page size for pulley #10285
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexcrichton
approved these changes
Feb 24, 2025
There was a problem hiding this comment.
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...
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Ah, I missed that the checks failed due to a CI failure on this one, so I haven't looked yet. I'll take a look in depth tomorrow, hopefully won't be too hard to resolve but I'll reach out if it's not making sense. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-actions
bot
added
the
wasmtime:api
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 ```
For the case that we construct a CodeMemory with a custom_code_memory implementation but we don't end up publishing memory, do not try to unpublish the memory. This could happen in a few cases: - The .text didn't require being made executable - There's an error or other circumstances cause the CodeMemory to be dropped prior to publish being called. Within the context of the .text size optimizations for pulley, this caused CI failures for a default pulley target as the errant unpublish was now being called on non-aligned memory. Prior to the .text optimizations there was still an mprotect being called to change bits on the .text region which just happened to be benign.
…ishing Changing the placement of published or introducing new state could be other ways of addressing the problem of doing a custom unpublish but those approaches would potentially create other issues with avoiding duplicate relocations. Given where this code would be used and the ability for the `custom_code_memory` to provide their own protections, this seems very much so Good Enough and gets the tests passing again while providing more reasonable behavior for user code.
posborne
force-pushed
the
reduce-cwasm-padding-pulley
branch
from
5a93a46 to
9e27cf0
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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=nI saw a reduction in the compiled wasm ELF from 421KB to 189KB with the sections no longer showing as being padded out significantly: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:
I'm also open to any feedback regarding if there is any additional testing that should be added in tree for this patch