Skip to content
This repository was archived by the owner on Nov 3, 2021. It is now read-only.

Update Overview.md for element segments #39

Merged
merged 1 commit into from
Nov 20, 2018
Merged

Update Overview.md for element segments #39

merged 1 commit into from
Nov 20, 2018

Conversation

binji
Copy link
Member

@binji binji commented Nov 9, 2018

Passive element segments now include an element type, and have a sequence of expressions instead of function indices.

Passive element segments now include an element type, and have a sequence of expressions instead of function indices.
| `0xd0 0x0b` | `ref.null end` | Returns a null reference |
| `0xd2 varuint32 0x0b` | `ref.func $funcidx end` | Returns a reference to function `$funcidx` |

TODO: coordinate with other proposals to determine the binary encoding for `ref.null` and `ref.func`.
Copy link
Contributor

Choose a reason for hiding this comment

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

The way the present proposal is written, passive element segments are unusable until we finish some future proposals. Ref.null comes from the reftypes proposal and is not yet ready; ref.func is not even in the reftypes proposal, and we don't know when we'll have it or even precisely what kind of form it will eventually take.

While we're waiting for that, if we want to initialize some part of a table that can't have an active segment, we will have to resort to an auxiliary table that does have an active segment + table.copy. This works but is hardly elegant; it incurs extra run-time overhead and the auxiliary table will be hanging around for the duration of the execution.

IMO a better way forward is to allow for (by means of more flags no doubt) a passive segment to have a type that can default to anyfunc, and if it defaults to anyfunc the element values of the segment could be as for active segments: function indices.

Copy link
Member Author

Choose a reason for hiding this comment

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

The way the present proposal is written, passive element segments are unusable until we finish some future proposals. Ref.null comes from the reftypes proposal and is not yet ready; ref.func is not even in the reftypes proposal, and we don't know when we'll have it or even precisely what kind of form it will eventually take.

I see what you mean, but as @conrad-watt mentioned, we could allow these instructions as an alternate encoding of function indices for now, with a validation failure if they're ever given a table that isn't anyfunc. We could even remove ref.null, since that isn't possible to specify with function indices.

The only concern I have about that is that we will have fixed the opcode for ref.func. But it seems the rest of the structure of that instruction should be OK. Or do you think we'll want to change the immediates/stack signature somehow?

IMO a better way forward is to allow for (by means of more flags no doubt) a passive segment to have a type that can default to anyfunc, and if it defaults to anyfunc the element values of the segment could be as for active segments: function indices.

Right, that's similar to what I suggested here: #27 (comment)

Personally I'm OK with this too, but I see the appeal of using a general form for passive element segments too.

xtuc pushed a commit to xtuc/v8 that referenced this pull request Nov 19, 2018
See the WebAssembly bulk memory proposal here:
https://github.com/WebAssembly/bulk-memory-operations

This initial CL adds a wasm experimental flag:
`--experimental-wasm-bulk-memory`, and also parsing of passive segments.

A passive segment is one that is not copied into the table/memory on
instantiation, but instead later via the `{table,memory}.init`
instructions.

The binary format of passive data segments is unlikely to change, but
the format for passive element segments may change (see
WebAssembly/bulk-memory-operations#39).

Bug: v8:7747
Change-Id: I2a7fb9bc7648a722a8c4aab4185c68d3d0843858
Reviewed-on: https://chromium-review.googlesource.com/c/1330015
Commit-Queue: Ben Smith <[email protected]>
Reviewed-by: Andreas Haas <[email protected]>
Cr-Commit-Position: refs/heads/master@{#57451}
| size | `varuint32` | always | size of `data` (in bytes) |
| data | `bytes` | always | sequence of `size` bytes |

Another way of looking at it:
Copy link
Member

Choose a reason for hiding this comment

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

Is this stray?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was, added some additional explanation in the new PR #41.

@binji binji merged commit 1901e56 into master Nov 20, 2018
@binji
Copy link
Member Author

binji commented Nov 20, 2018

Accidentally merged this, I've rolled back master and moved this to a new PR here: #41

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants