Lars T Hansen [:lth] Assignee
	Description • 8 years ago

The meaning of the count argument has changed: it defaults to +Infinity, not 0.

Note, the futex functions may go away, we should not implement the change until that is settled.

	Florian Scholz (Open Web Docs)
	Comment 1 • 8 years ago

Dev docs to be updated once this is resolved:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics/futexWake
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics/futexWakeOrRequeue

	Lars T Hansen [:lth] Assignee
	Comment 2 • 8 years ago

The futex functions will not go away (well, futexWakeOrRequeue may but futexWake will not) so we should fix this.

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 4 • 8 years ago

Comment on attachment 8734392 [details] [diff] [review]
count argument of futexWake defaults to +Infinity

Review of attachment 8734392 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: js/src/builtin/AtomicsObject.cpp
@@ +901,5 @@
>      uint32_t offset1;
>      if (!GetTypedArrayIndex(cx, idx1v, view, &offset1))
>          return false;
>      double count;
> +    if (countv.isUndefined()) {

Overly-annoying reviewer would suggest to share this code in a static function; reviewee does not have to listen, though. (I'm reading futexWakeOrRequeue might disappear anyway)

::: js/src/tests/shell/futex.js
@@ +9,5 @@
>      reportCompare(true,true);
>      quit(0);
>  }
>  
> +var DEBUG = true;

nit: probably unintended change?

@@ +107,5 @@
> +
> +evalInWorker(`
> +var mem = new Int32Array(getSharedArrayBuffer());
> +sleep(1);
> +Atomics.futexWake(mem, 0);	// Last argument should default to +Infinity

If you had the call to futexWait before the call to futexWake, could you also assertEq(Atomics.futexWake(mem, 0), 1); ? (if futexWait can be run on a worker too)

	Lars T Hansen [:lth] Assignee
	Comment 5 • 8 years ago

(In reply to Benjamin Bouvier [:bbouvier] from comment #4)
> Comment on attachment 8734392 [details] [diff] [review]
> count argument of futexWake defaults to +Infinity
> 
> Review of attachment 8734392 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!
> 
> ::: js/src/builtin/AtomicsObject.cpp
> @@ +901,5 @@
> >      uint32_t offset1;
> >      if (!GetTypedArrayIndex(cx, idx1v, view, &offset1))
> >          return false;
> >      double count;
> > +    if (countv.isUndefined()) {
> 
> Overly-annoying reviewer would suggest to share this code in a static
> function; reviewee does not have to listen, though. (I'm reading
> futexWakeOrRequeue might disappear anyway)

Reviewee is not annoyed but notes that futexWakeOrRequeue will go away.

> ::: js/src/tests/shell/futex.js
> @@ +9,5 @@
> >      reportCompare(true,true);
> >      quit(0);
> >  }
> >  
> > +var DEBUG = true;

Oops.

> @@ +107,5 @@
> > +
> > +evalInWorker(`
> > +var mem = new Int32Array(getSharedArrayBuffer());
> > +sleep(1);
> > +Atomics.futexWake(mem, 0);	// Last argument should default to +Infinity
> 
> If you had the call to futexWait before the call to futexWake, could you
> also assertEq(Atomics.futexWake(mem, 0), 1); ? (if futexWait can be run on a
> worker too)

That would be a nice improvement.  "Before" is not always so easy to arrange but I'll look into it.

	Ryan VanderMeulen [:RyanVM]
	Comment 6 • 8 years ago

This landed with the wrong bug number in the commit message. Please double-check that in the future before pushing.
https://hg.mozilla.org/mozilla-central/rev/df3ddeee1647

	Florian Scholz (Open Web Docs)
	Comment 7 • 8 years ago

Updated https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics/futexWake

Not going to add any compatibility note as this is an experimental API anyway and we are shipping the old behavior for 2 releases only (will this be uplifted?).