Make sbrk(0) deterministic

[mikevoronov]

The Wasm spec doesn't specify explicitly the behaviour of memory.grow 0. And because of

The memory.grow instruction is non-deterministic. It may either succeed, returning the old memory size
sz , or fail, returning −1. Failure must occur if the referenced memory instance has a maximum size defined that
would be exceeded. However, failure can occur in other cases as well. In practice, the choice depends on the
resources available to the embedder.

memory.grow 0 could potentially be non-deterministic. From the other side, WASI dlmalloc heavily relies on it. So, it seems that it is better to make sbrk(0) deterministic.

Also memory.size is much faster on many execution environments, since memory could shared and some code with locking could be called (like in v8).

[sunfishcode]

Good spot! That's a really subtle issue.

[sbc100]

Is there any fundamental reason why memory.grow 0 can't be just as fast as memory.size. They both need to handle the case where memory is shared, i would have though engines could trivially just check for the zero arg to do exactly the same things as memory.size in that case?

Did you observe it being slower in v8? (or in any specific engine?)

I agree this change makes sense, I just don't understand why an engine wouldn't do exactly the same thing internally?

[AndrewScheidecker]

IMO the problem here is that the WASM spec doesn't guarantee that memory.grow 0 is equivalent to memory.size.

[mikevoronov]

@sbc100 Maybe I wasn't fully correct. Of cause, they could just check that delta == 0 and then return current size. But in v8 source code they don't do this (https://github.com/v8/v8/blob/4b9b23521e6fd42373ebbcb20ebe03bf445494f9/src/wasm/wasm-objects.cc#L1240). So, I just meant that developers could forget to make this check. And this behaviour would be correct according to the standard.

@AndrewScheidecker I also think that the spec must specify this behaviour, but changing Wasm spec isn't so fast? Anyway, I think it worth to create an issue in the spec.

[sunfishcode]

Agreed; this a correctness issue. sbrk(0)'s interface is that it should return the current heap limit and never fail, while wasm's malloc.grow is permitted to fail even with a delta of 0.

The spec process does take time, though the other question here is whether the CG would be willing to cause existing conforming implementations to become non-conforming. An implementation which doesn't support memory.grow could just return -1 unconditionally today, and the change would make it non-conforming. If the CG finds that this compatibility concern is small enough to not worry about, wasi-libc probably wouldn't need to worry about it either, and we could just revert the patch as soon as there's a decision.