Wasmtime GitHub notifications bot (Aug 09 2022 at 23:39): Wasmtime GitHub notifications bot (Aug 09 2022 at 23:39):github-actions[bot] commented on issue #4667:
Subscribe to Label Action
cc @fitzgen
<details>
This issue or pull request has been labeled: "cranelift", "fuzzing"Thus the following users have been cc'd because of the following labels:
- fitzgen: fuzzing
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Aug 17 2022 at 10:03):afonso360 commented on issue #4667:
@jameysharp @cfallin I think this is ready, would you guys be able to review it?
Wasmtime GitHub notifications bot (Aug 17 2022 at 10:09):afonso360 edited a comment on issue #4667:
@jameysharp @cfallin I think this is ready, would you be able to review it?
Wasmtime GitHub notifications bot (Aug 17 2022 at 17:42):cfallin commented on issue #4667:
So far my only complaint is that the 16-character limit on function names forces changing a lot of tests, when I would think it'd be better to just increase the limit on name length
Historically at least, the limit was to keep the size of
ExternalNamereasonable; these are embedded and carried in the ir::Function and in "production" use, are twou32s only (theu0:0format) because the embedder is supposed to own names and Cranelift handles indices only. I wasn't around for that design decision and I'm not sure if it would make sense to support a boxed variant for the "one-off test function with an owned name" case; that would be the way to go, IMHO, rather than bumping the limit and inflating the fixed-size field.
Wasmtime GitHub notifications bot (Aug 17 2022 at 17:52):jameysharp commented on issue #4667:
Maybe the test-runner should keep a mapping between heap-allocated names and the "pair of u32" representation, just like an embedder would?
Wasmtime GitHub notifications bot (Aug 17 2022 at 17:55):afonso360 commented on issue #4667:
Maybe the test-runner should keep a mapping between heap-allocated names and the "pair of u32" representation, just like an embedder would?
I think this would be a good idea, if we can remove
TestCasefromExternalNameI think the struct size goes down, so it could potentially reduce memory usage and speedup some stuff!
Wasmtime GitHub notifications bot (Aug 17 2022 at 18:17):afonso360 edited a comment on issue #4667:
Maybe the test-runner should keep a mapping between heap-allocated names and the "pair of u32" representation, just like an embedder would?
I think this would be a good idea, if we can remove
TestCasefromExternalNameI think the struct size goes down, so it could potentially reduce memory usage and speedup some stuff!Edit: Had a look around the code, and we probably also need to change the parser and a few other things for this.
I think it would be best if it was a separate PR, I can put this on hold while I try to figure it out.
Wasmtime GitHub notifications bot (Aug 18 2022 at 16:05):jameysharp commented on issue #4667:
I think this would be a good idea, if we can remove
TestCasefromExternalNameI think the struct size goes down, so it could potentially reduce memory usage and speedup some stuff!I can confirm it's currently 20 bytes with
TestCase, and only 8 bytes without, so that sounds good!I've just done an experiment, reducing the length limit of test-case names to 6 bytes, so it fits inside an 8-byte
ExternalNamewithout having to make all the changes needed to actually removeTestCase. I'm runningvalgrind --tool=dhat target/release/wasmtime compileon the pulldown-cmark benchmark from Sightglass, with the wasmtime CLI built with--no-default-featuresto disable parallel compilation and other sources of nondeterminism.With that experimental setup, the smaller
ExternalNamemakes a positive difference, but it's... small. Total bytes allocated drops by 40,504 bytes out of about 171MiB. Maximum heap size drops by 1,096 bytes out of about 12MiB. And the program executes about 1 million fewer instructions, out of close to 2 billion.So I think this is totally worth doing, but... don't get your hopes up for much improvement in memory usage or compile speed. :grin: I was really curious which way this would go, and it was fun doing the quick experiment.
Wasmtime GitHub notifications bot (Aug 23 2022 at 18:11):afonso360 commented on issue #4667:
With that experimental setup, the smaller ExternalName makes a positive difference, but it's... small. Total bytes allocated drops by 40,504 bytes out of about 171MiB. Maximum heap size drops by 1,096 bytes out of about 12MiB. And the program executes about 1 million fewer instructions, out of close to 2 billion.
That's... not as much as I hoped for :big_smile:. Thanks for checking anyway!
I started working on this refactor a couple days ago (in progress branch), but it's going to touch a lot of stuff and I don't think its going to be ready anytime soon.
@jameysharp Would it be ok to merge this with the test renames? I'd like to build on this to fix https://github.com/bytecodealliance/wasmtime/issues/4758 but its going to take a while if I have to do the refactor first and I'd prefer to use that time to fix the other fuzz issues.
Wasmtime GitHub notifications bot (Aug 23 2022 at 19:53):jameysharp commented on issue #4667:
I was hesitant, but actually yeah, renaming the test functions to fit within the limit is fine. Let's just plan that later we'll do a partial revert to get the more descriptive test names back.
One alternative that comes to mind is to leave the
TestCaseoption inExternalName, but make it aBox<[u8]>instead. ThenExternalNamehas the same size as it does now but hopefully it's easier to switch things over.I'm happy enough either way though.
That said, I haven't had a chance to review this more carefully yet, so I'm not ready to merge it. Maybe @cfallin can get it into his review queue but he's got a lot on his plate. So maybe we can chat in #4758 about whether there are easier ways to fix the immediate bug there.
Wasmtime GitHub notifications bot (Aug 24 2022 at 09:13):afonso360 commented on issue #4667:
I've filed https://github.com/bytecodealliance/wasmtime/issues/4766 to track this refactor.
One alternative that comes to mind is to leave the TestCase option in ExternalName, but make it a Box<[u8]> instead. Then ExternalName has the same size as it does now but hopefully it's easier to switch things over.
Thanks for picking this up! I didn't even consider that as an option, but its a great middle step and solves the immediate issue.
That said, I haven't had a chance to review this more carefully yet, so I'm not ready to merge it. Maybe @cfallin can get it into his review queue but he's got a lot on his plate. So maybe we can chat in https://github.com/bytecodealliance/wasmtime/issues/4758 about whether there are easier ways to fix the immediate bug there.
:+1:
Wasmtime GitHub notifications bot (Aug 26 2022 at 21:53):jameysharp commented on issue #4667:
Well, darn. This seems to have broken
cranelift-fuzzgen, and I don't understand why. But 20 out of 50 cases I tested from my existing corpus crash now, and it looks like they're all with one of these two messages:thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: Undeclared function u0:0 is referenced by u0:1!', fuzz/fuzz_targets/cranelift-fuzzgen.rs:73:53 thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: Duplicate function with name u0:1 found!', fuzz/fuzz_targets/cranelift-fuzzgen.rs:76:10
Wasmtime GitHub notifications bot (Aug 26 2022 at 22:56):jameysharp commented on issue #4667:
Okay, I'm reasonably convinced that "Undeclared function u0:0 is referenced by u0:1" has the same root cause as #4757 and #4758, although it's a different symptom. Previously these undefined functions caused failures if the interpreter tried to actually call them, or if the JIT tried to compile a call instruction referencing them. Now the new code also fails if there's a function signature declared, even if that function is never called.
I've opened PR #4795 for the other error. It's a small fix.
Last updated: Nov 22 2024 at 17:03 UTC