Stream: git-wasmtime

Topic: wasmtime / PR #4764 Don't limit ExternalName::TestName le...

Wasmtime GitHub notifications bot (Aug 24 2022 at 00:01):

jameysharp opened PR #4764 from box-testname to main:

Following up on discussion in #4667, here's a way to avoid making sweeping changes while still using reasonable names in filetests. @afonso360, how does this look to you? @cfallin, do the numbers here sound okay to you?

In order to keep the ExternalName enum small, the TestcaseName struct was limited to 17 bytes: a 1 byte length and a 16 byte buffer. Due to alignment, that made ExternalName 20 bytes.

That fixed-size buffer means that the names of functions in Cranelift filetests are truncated to fit, which limits our ability to give tests meaningful names. And I think meaningful names are important in tests.

This patch replaces the inline TestcaseName buffer with a heap-allocated slice. We don't care about performance for test names, so an indirection out to the heap is fine in that case. But we do care somewhat about the size of ExternalName when it's used during compiles.

On 64-bit systems, Box<[u8]> is 16 bytes, so TestcaseName gets one byte smaller. Unfortunately, its alignment is 8 bytes, so ExternalName grows from 20 to 24 bytes.

According to valgrind --tool=dhat, this change has very little effect on compiler performance. Building wasmtime with --no-default-features --release, and compiling the pulldown-cmark benchmark from Sightglass, I measured these differences between main and this patch:

• total number of allocations didn't change (ExternalName::TestCase is not used in normal compiles)
• 592 more bytes allocated over the process lifetime, out of 171.5MiB
• 320 more bytes allocated at peak heap size, out of 12MiB
• 0.24% more instructions executed
• 16,987 more bytes written
• 12,120 _fewer_ bytes read

Wasmtime GitHub notifications bot (Aug 24 2022 at 00:01):

jameysharp requested cfallin for a review on PR #4764.

Wasmtime GitHub notifications bot (Aug 24 2022 at 00:33):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 24 2022 at 04:17):

jameysharp merged PR #4764.

Last updated: Oct 23 2024 at 20:03 UTC