Conversation
lukewagner
left a comment
There was a problem hiding this comment.
Thanks! This is looking generally good, a few comments:
| `none` case of an optional immediate.) | ||
| * 🔧 for fixed-sized lists the length of the list must be larger than 0 to pass | ||
| validation. | ||
| * 🔧 for fixed-sized lists (`0x67`) the length of the list must be larger than |
There was a problem hiding this comment.
Pre-existing, but it looks like the grammar already covers this twice: once by using <u32> (unsigned) and once with the (if maxlen > 0). We could also remove the (if maxlen > 0). But should we specify a maximum for maxlen?
There was a problem hiding this comment.
For u32, zero is valid and the maxlen > 0 checks forbids zero. What would be a legitimate upper bound... i32::MAX?
There was a problem hiding this comment.
There's already (just recently added) a MAX_LIST_BYTE_LENGTH = 228-1. That's just bytes, but even still, it seems like a reasonable upper bound.
There was a problem hiding this comment.
Ok, defined that bound in BINARY.md but where would we check this in definitions.py ?
|
Associated with Implement WIT Fixed-Length-Lists |
Cherry-picked the essence of cpetig's commit d2874eb from https://github.com/cpetig/component-model/tree/bounded-lists, adapted to the current codebase (ptr_type/opts threading, updated class names). Bounded strings are intentionally excluded. Co-authored-by: Christof Petig <christof.petig@arcor.de>
- Add trap_if(actual_len > maybe_length) to lift_flat_list, mirroring the existing trap in load_list's heap path - Add over-length trap tests for both flat and heap lifting - Add alignment test for bounded list of U32 (verifies 3-byte padding after U8 length prefix)
- fix memory bounds checking - improve integration with existing list load/store code - fix indentation - avoid default argument - more readable load/store recipe
Replace the expanded-element flat representation with a pointer-based one for direct fixed and bounded length lists. Add contains_direct_list and type_has_direct_list_result helpers to detect when result types contain such lists, forcing memory-based returns. Update test expectations and add new roundtrip tests for the pointer-based flat ABI.
|
Here is another shot at an ABI tweak for efficient passing of fixed/bounded lists. It is already implemented in the commit I just pushed. @cpetig 's ideas with refinement by discussion, my write-up; more versatile / less invasive compared to my previous proposal. Flat ABI for Fixed and Bounded Length ListsMotivationThe current flat ABI for fixed length lists expands all N element slots If the current approach was naively adopted for bounded length lists A second, independent motivation is that fixed and bounded length lists This proposal replaces the expanded-element flat representation with a Flat representation
Memory representation
bounded actual len: smallest integer type that can represent values up to (including) N Lowering and LiftingParametersBelow explanation is for bounded length lists which carry a runtime length. If all core params fit within
ResultsMemory-passing is enforced when even a single direct fixed/bounded length list is in the result type -
Fused allocationWhen multiple direct lists exist among parameters and passing flat, the host might service |
|
Thanks for the clear write-up (and sorry for the delay replying)! That all sounds pretty good; just one suggestion: if a component exports a function with type |
I addressed that in the trailing "fused allocation" section - yes it should be done but we need not guarantee that by design. In practice, the host can alloc one big chunk for everything and hand out pointers to subobjects (list1, list2, list3, etc. correctly aligned and sized of course). The allocation will continue to be owned by the host.
Passing all params in a param-area, even if callee side only, feels a bit inefficient. We should try to utilize registers for non-list value passing. But that's just my 2 cents... PS: |
I think the host can do that when given a list, b/c the host can do whatever, but if we're talking about lowering a list into the guest, the
I suppose, but I would expect this to be amortized by the cost of at least 1 |
Sorry if I'm repeating myself. I might be missing some crucial C-M insight...
What obligation would make fusion impossible? |
My understanding is that this logic is already needed for the argument area whenever calling into the guest with more than 16 flat parameters. We simply start using this linear memory argument area earlier for spilling over the fixed/bounded list data when the rest of the arguments still fit into registers. |
|
(see WebAssembly/WASI#944 for previous status updates and reasoning) I implemented more of this logic inside wasmtime and it needed a larger restructuring in the environ crate - because now you should allocate memory across all arguments (already default when not flattened) and pass flattened arguments pointing there. So the calculation of flattened properties needs to pass this information on. Still working on this. |
|
Opus 5.5 pointed out an interesting inconsistency in my implementation: Does a guest imported Felix wrote "Memory-passing is enforced when even a single direct fixed/bounded length list is in the result type - that's a new second switchover reason beyond exceeding MAX_FLAT_RESULTS." This avoids running into the situation that an out pointer to caller-allocated memory is passed as an additional argument while still returning values, thus trading a special case (asymmetry to argument handling) for another one (unnecessary return value). The option I had in mind was passing a "scratch buffer" to the function which would be used by the host to place array elements into. So an out buffer parameter which only is present if fixed-length and bounded lists are part of the flattened result. If we add multi-value to the mix, this gets more interesting, returning a @lukewagner How do you feel about this? Do you value symmetry with argument handling or simplicity (no strange scratch buffer) higher? Felix and me found that both have value. (Although in writing this I realized that the scratch buffer feels more of a strange new concept needing explanation, in this case it would be 20+4 integers versus 1+20+4+1 integers for the all-in-memory case) PS: This proposal got rid of some special handling in the wasmtime flattening code, while introducing data structure complications for argument handling. But to me it feels more right than the simpler "consider arrays as tuples" existing strategy. |
|
I hope I haven't misunderstood anything, but I don't understand why bounded lists should be any different from plain old arbitrary-length lists at the ABI level. Putting fixed-length lists aside for a moment, the motivation for bounded lists is to avoid runtime allocations. But that just means the bindings generator has to know the maximum size of the incoming data so it can preallocate a sufficiently large chunk of memory. The actual data passed via the ABI could look exactly the same as it does today. Yes, I realize you could pass a bounded list's data in params, but who is out here passing around bounded lists of length 16 or less? And that's assuming the list's elem type is one param; in practice I'd imagine your list bound would likely have to be 8 or 4 or 2, and who is passing around lists with a maximum size of 2? So to me it just seems like bounded lists should in fact just be normal lists at the ABI level. Just pass them in memory, since 99% of the time the maximum length will exceed MAX_FLAT_PARAMS. As for fixed-length lists, I understand that you'll have to copy from params into memory before indexing the list, but that seems...super incredibly normal. And the alternative is to call
As I understand, option 3 is being proposed, but like...why? You're guaranteed to realloc now, and most of the flattening logic will still need to assume tuples, and you need to define "fusion". Why not just go with option 1, which is simple and probably fastest in all common cases? |
Add bounded lists (
list<T, ..N>)Closes #385.