Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fallible struct/array allocation #87

Closed
eqrion opened this issue Apr 2, 2020 · 9 comments
Closed

Fallible struct/array allocation #87

eqrion opened this issue Apr 2, 2020 · 9 comments

Comments

@eqrion
Copy link
Contributor

eqrion commented Apr 2, 2020

The struct.new and array.new instructions are specified in MVP.md to return ref T which is non-nullible. This seems to prevent an implementation from returning null in the case where it failed to allocate the object, and would leave implementations with no option but to trap.

All instructions that allocate so far (memory.grow/table.grow) are fallible currently, and so it seems useful for struct/array allocation to support this as well, which would imply returning an optref T.

@RossTate
Copy link
Contributor

RossTate commented Apr 2, 2020

This is a good point, though I don't think the appropriate fix is to return null. Unlike memory.grow/table.grow which are coarse-grained allocations, struct.new/array.new are fine-grained allocations that will be done very frequently. This makes branching to detect an exceptional circumstance very expensive. So something like exceptional control flow would likely be a better fit. Unfortunately the current exceptions proposal requires dynamically allocating memory on the heap, and so the process of throwing an out-of-heap-memory exception would itself throw an out-of-heap-memory exception.

@lars-t-hansen
Copy link
Contributor

Unfortunately the current exceptions proposal requires dynamically allocating memory on the heap, and so the process of throwing an out-of-heap-memory exception would itself throw an out-of-heap-memory exception.

Pragmatically, when a "small" allocation request fails it is frequently better just to abort -- your program is about to go down in flames anyway. When a "large" allocation request fails it is often desirable to handle it because it is often possible to take evasive action. Indeed memory.grow and table.grow are within this latter category of requests. For the large-object case an exception object allocation would likely succeed, too.

For the MVA I would advocate aborting (OOM error) on allocation failure for struct.new and array.new, these operations can continue to return non-nullable values. For post-MVA we will for sure want a fallible array.new variant, and maybe a fallible struct.new variant since some structures can be large when they contain inline constant-size arrays (as they eventually will). I tend to think exceptions are a poor fit for signaling allocation failures because you almost always want to handle the failure fairly locally, and the type system will save us from NPEs anyway, but we don't need to settle that now.

@rossberg
Copy link
Member

rossberg commented Apr 3, 2020

Agreed with @lars-t-hansen. For now, this assumes a trap when allocation fails.

@eqrion
Copy link
Contributor Author

eqrion commented Apr 3, 2020

Interesting, the likelihood that small allocation failures imply a total program failure is a good point.

However, I think that 'abort on allocation failure' can still be achieved with optref constructors as the first use of the object will trap. struct/array.get/set all take optref and will trap on null.

If the program wished to force an immediate trap, it could insert a ref.as_non_null immediately after construction. Or else they could also insert a br_on_null if they wished to handle the error. A throw_on_null instruction could also be created for languages that wish to handle OOMs with exceptions.

I understand that it's currently unlikely that an allocation failure will be recoverable as it's likely to be small. However, this design would have flexibility for future cases that may need it without adding new instructions down the road.

@RossTate
Copy link
Contributor

RossTate commented Apr 3, 2020

I should say that I agree with @lars-t-hansen and @rossberg above. I did not mean to suggest that the instructions should be changed. Rather, I was explaining why null pointers are not a good solution, and why currently there is no good alternative solution.

@eqrion, I think what we are saying is that we hear your concern and agree that it should be addressed, but that since it will generally be hard to recover from such a situation and that it will take time to come up with a good general-purpose way to enable such recovery, a solution to this problem is better deferred to after the MVP.

@eqrion
Copy link
Contributor Author

eqrion commented Apr 3, 2020

@eqrion, I think what we are saying is that we hear your concern and agree that it should be addressed, but that since it will generally be hard to recover from such a situation and that it will take time to come up with a good general-purpose way to enable such recovery, a solution to this problem is better deferred to after the MVP.

I agree that this is a challenging situation to recover from and that it's unclear that languages would do anything but 'abort on failure' here.

That being said, what is the downside to returning an optref from struct/array.new?

Languages would not be required to emit any more code to have 'abort on failure' semantics as all of the accessor instructions operate on optref anyway and will trap on null.

Additionally, if you are already relying on accessor instructions trapping on null, you could eliminate some overhead from struct/array.new by it no longer being a trap site (the amount is a bit dubious however).

If the design was changed to discourage null usage by having accessors operate on ref, then I don't think this change is a good idea. I also don't know existing language semantics around allocation failure. If they require abort on failure at the construction site, then this probably isn't a good design either.

@RossTate
Copy link
Contributor

RossTate commented Apr 3, 2020

That being said, what is the downside to returning an optref from struct/array.new?

I see three downsides. 1 is that WebAssembly cares very much about program size, so it wants the common case to be concisely expressible. Changing to optref requires the common case to use more instructions, e.g. ref.as_non_null. 2 is that the optref design causes, at least at the low-level, more branching, which is performance overhead. 3 is that it is unclear that optref is the best solution to the problem, imposing a backwards-compatibility constraint if we find a better solution once we've had time to investigate the problem more thoroughly. (Point 3 is more a reminder that there is maintenance cost for each instruction we have, and not specific to optref.)

@eqrion
Copy link
Contributor Author

eqrion commented Apr 10, 2020

I see three downsides. 1 is that WebAssembly cares very much about program size, so it wants the common case to be concisely expressible. Changing to optref requires the common case to use more instructions, e.g. ref.as_non_null.

That depends on the common case. As I said, if a language runtime wished to allow the trap on a failed allocation to be delayed to the first use of the allocation, then this has the exact same program size.

However, from offline discussions I've come to see that it's unlikely that language runtimes would want delayed traps. As that's the case, I'm fine with leaving the design as is for now.

@binji
Copy link
Member

binji commented May 27, 2020

Sounds like this question is resolved; closing. Feel free to reopen in the future, if new information comes to light.

@binji binji closed this as completed May 27, 2020
rossberg pushed a commit that referenced this issue Feb 24, 2021
Change the test generators to use `ref.func` and remove `passive`.

At some point we'll want to remove the generators, but for let's try to
maintain them.
rossberg added a commit that referenced this issue Feb 24, 2021
Per the vote on #69, this PR removes subtyping from the proposal. List of changes:

* Syntax:
  - remove `nullref` type
  - rename `anyref` type to `externref`
  - extend `ref.null` and `ref.is_null` instructions with new immediate of the form `func` or `extern` (this will later have to generalise to a `constype` per the [typed references proposal](https://github.com/WebAssembly/function-references))
* Typing rules:
  - `ref.null`, `ref.is_null`: determine reference type based on new immediate
  - `select`, `call_indirect`, `table.copy`, `table.init`: drop subtyping
  - `br_table`: revert to rule requiring same label types
  - `elem` segment: drop subtyping
  - `global` import: drop subtyping (link time)
* Remove subtyping rules and bottom type.
* Revert typing algorithm (interpreter and spec appendix).
* JS API:
  - remove `"nullref"`
  - rename `"anyref"` to `"externref"`
* Scripts:
  - rename `ref` result to `ref.extern`
  - rename `ref.host` value to `ref.extern`
  - drop subtyping from invocation type check
* JS translation:
  - extend harness with separate eq functions for each ref type
* Adjust tests:
  - apply syntax changes
  - remove tests for subtyping
  - change tests exercising subtyping in other ways
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants