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

The result of the wake operation #72

Open
lars-t-hansen opened this issue Oct 20, 2017 · 22 comments
Open

The result of the wake operation #72

lars-t-hansen opened this issue Oct 20, 2017 · 22 comments

Comments

@lars-t-hansen
Copy link

The return type of wake is currently int32 (according to the proposal text it is actually the wake count "converted" to int32, which is an underspecification but let's assume standard mod-2^32 wrapping was intended). This is fine for an explicit wake count but if we pass -1 as the count to wake then there's a risk that information is lost:

  • On 64-bit systems we could imagine having more than 2^32 threads, OS ulimits allowing, since once we move away from workers to real threads we could conceivably be creating a very large number of them without exhausting other resources
  • If the Atomics.waitAsync proposal before TC39 is adopted then it will be possible to create a very large number of waiters irrespective of OS limits on threads, and these will be visible to wasm's wake.

Either way the wake count can exceed 2^32 and information can be lost. We can argue that especially the first case is theoretical, and that an int64 will suffice for the second case on 64-bit systems, or that this overflow really does not matter one whit, because, the programmer had it coming, but perhaps the issue here is that we should be able to signal "lost information" in a sensible way in this API.

I suggest that we change the spec so that if the result in the wake algorithm is greater than INT32_MAX then we return a cookie such as -1 or INT32_MIN or ..., or we clamp to INT32_MAX, to allow programs that care about this to detect the problem. This seems slightly more appealing than changing the return type to int64. (In JS the return type is Number, which is eventually lossy but provides a clear signal that information is lost.)

cc @bnjbvr

@jfbastien
Copy link
Member

I'm not sure I understand why you think changing to i64 is a bad idea. Could you elaborate?

@lars-t-hansen
Copy link
Author

Well. I did not say i64 was a "bad idea", I said a well-defined overflow semantics seemed "slightly more appealing", clearly a matter of taste. i64 just postpones the problem: I, for one, look forward to the glorious 128-bit future.

Anyway, I would not object to i64 if everyone thinks that takes care of the problem. Maybe worthy of discussion at the next meeting?

@jfbastien
Copy link
Member

Yeah that sounds fine. I don't see anything bad with i64, except maybe a slight incompatibility with Number on the JS side (and JS could return inf on overflow...)

@binji
Copy link
Member

binji commented Oct 20, 2017

I dunno, I'm having a hard time imagining a scenario where we'll wake 4 billion threads (or even async waiters). But either way, I'm fine with either solution. Let's bring this up at the CG and see what folks think.

@lars-t-hansen
Copy link
Author

If we think it's a realistic possibility that more than 2^32 waiters can be woken then the count argument should also be i64.

@jfbastien
Copy link
Member

I think it's unrealistic, but at the same time I don't see a reason that we should restrict to i32 when we have native i64 support.

binji added a commit that referenced this issue Oct 26, 2017
@lars-t-hansen
Copy link
Author

The change to the spec still just says "converted to i64". Maybe there's a canonical definition of that word that I've missed; barring that, the nature of the conversion might usefully be specified.

A poll for this issue is not currently on the agenda for the next CG.

@binji
Copy link
Member

binji commented Oct 30, 2017

Added poll.

The change to the spec still just says "converted to i64". Maybe there's a canonical definition of that word that I've missed; barring that, the nature of the conversion might usefully be specified.

Maybe it should just say "represented as i64" instead? The definition currently piggybacks on the result from Atomics.wake which is the JS Number. The only operations on it are to set to let n be 0 and to add 1 to n. Once it gets to 2**53 that increment won't work anyway, so we'll only have to represent values in the range [0, 2**53].

Or does the ES spec operate on infinite precision numbers? If so, we have a less satisfying result already since the wake counts > 2**53 won't be precise. How about this: we specify that the value is converted using similar behavior as i64.trunc_u:sat/f64 in the nontrapping-float-to-int proposal?

@lars-t-hansen
Copy link
Author

If we end up going with int64 then I think we should stay away from referencing Atomics.wake directly since it uses a double, a necessary compromise for JS. Instead we should spec that the wasm implementation of wake should be precise up to INT64_MAX (and saturating after that :); we should not use a double at any point.

Then JS is free to lose accuracy past 2^53 since it does not yet have int64 :)

@jfbastien
Copy link
Member

At the meeting yesterday we discussed trapping instead of saturating. Shouldn't JS do the same as well?

@lars-t-hansen
Copy link
Author

It's perhaps worth bringing up with the committee as an adjustment to the JS API, though to be fair it is already possible to detect overflow unambiguously and naturally in JS (the value being a double). Also, JS is traditionally less keen on signaling errors.

Or were you thinking, JS should trap at INT32_MAX like Wasm does?

@binji
Copy link
Member

binji commented Nov 2, 2017

In the CG we discussed bringing up throwing an exception on INT32_MAX in JS too.

@jfbastien
Copy link
Member

I thought we discussed throwing "on overflow", not necessarily at INT32_MAX 😁

@binji
Copy link
Member

binji commented Nov 2, 2017

True, I suppose we didn't really say where. Though we were undecided as a group about whether to switch wake count to i64 or keep at i32.

@jfbastien
Copy link
Member

Right, what I'm thinking is you're free to trap on i32 overflow, or on i53 overflow.

@lars-t-hansen
Copy link
Author

Right, what I'm thinking is you're free to trap on i32 overflow, or on i53 overflow.

Who is "you" in that sentence? Is it "TC39 is free to decide whether to trap on i32 oflo or i53 oflo" or is it "implementations are free to decide whether to trap on i32 oflo or i53 oflo"? The latter seems like a non-starter; the former just takes us full circle to whether ES should have behavior compatible with Wasm or not.

@jfbastien
Copy link
Member

The WebAssembly CG didn't express a preference. Why would trap on i53 overflow be a no-go? This is from wake, which can check for that overflow easily, no?

@lars-t-hansen
Copy link
Author

Summarizing the meeting notes: The CG has a "slight preference for staying with i32", with a clear preference for trapping on overflow.

There's also an expressed desire to sync with TC39 but nobody knows what TC39 will do. I expect resistance to trapping on overflow, because it's just not very ES.

FWIW, pending Firefox patches have a 32-bit count/result as of now. I'll add trapping on overflow. It'll stay that way until this issue has been resolved.

@jfbastien, trapping on i53 overflow is a non-starter if the result is i32. If the result is i64 it'll work fine, of course.

@jfbastien
Copy link
Member

The assumption was that we had a Number on the JS side.

@binji
Copy link
Member

binji commented Nov 28, 2017

We discussed this in the Nov 28 CG, and decided to go back to 32-bit w/ trapping on overflow.

@conrad-watt
Copy link
Collaborator

To resurrect this, if the wake count overflows, are the other threads actually woken, even if the wake operation itself traps?

@lars-t-hansen
Copy link
Author

Given that wake must return the number of threads woken, there must already be enough machinery in place to account for waiting threads and to ensure that threads are woken in a predictable way (also there is fifo ordering of wakeups). I expect it is no hardship to require that no other threads are woken if wake traps due to overflow.

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

4 participants