d8 Worker test of SharedArrayBuffer transferring

d8 Worker test of SharedArrayBuffer transferring

BUG=chromium:497295
R=jarin@chromium.org
LOG=n

Committed: https://crrev.com/686e675734579e393fa813c745716584a90e9e49
Cr-Commit-Position: refs/heads/master@{#29394}

[Jarin, 2015-06-30 17:42:43]

lgtm once the comments are addressed.

Non-blocking observation: initializing Worker with a function without the
closure looks confusing. I am wondering whether we should not create workers
with a string. That at least removes the element of surprise. If one really
wants to do without the string, I guess you can still do something like

function f() { ... }
w = new Worker(f.toString() + "; f();");

https://codereview.chromium.org/1216023003/diff/20001/test/mjsunit/d8-worker-...
File test/mjsunit/d8-worker-sharedarraybuffer.js (right):

https://codereview.chromium.org/1216023003/diff/20001/test/mjsunit/d8-worker-...
test/mjsunit/d8-worker-sharedarraybuffer.js:31: function f() {
I do not think this is doing what you think it's doing. E.g.

var x = false;
if (x) function f() { print("f"); }
f();

still happily prints f.

You really have to say 
var f = function () { ... }

Even then I am not entirely sure why this has to be conditioned on this.Worker
being defined?

https://codereview.chromium.org/1216023003/diff/20001/test/mjsunit/d8-worker-...
test/mjsunit/d8-worker-sharedarraybuffer.js:36: throw new
Error("SharedArrayBuffer transfer byteLength");
Nit: Could we get the braces here and below? (In general, we like braces if the
body is not on the same line.)

https://codereview.chromium.org/1216023003/diff/20001/test/mjsunit/d8-worker-...
test/mjsunit/d8-worker-sharedarraybuffer.js:55: assertEquals(16,
sab.byteLength);  // ArrayBuffer should not neutered.
In the comment: ... should not *be* neutered.

https://codereview.chromium.org/1216023003/diff/20001/test/mjsunit/d8-worker-...
test/mjsunit/d8-worker-sharedarraybuffer.js:59: while ((ta0 = Atomics.load(ta,
0)) == 0);
"while (<condition>);" is not recommended by the Chromium C++ style guide (and
we are trying to stick to the C++ style guide even for JS).

Either do "while (<condition>) continue;" or "while (<condition>) {}".

https://codereview.chromium.org/1216023003/diff/20001/test/mjsunit/d8-worker-...
test/mjsunit/d8-worker-sharedarraybuffer.js:63: w.terminate();
Could we also check that the array has not been neutered after termination?

[binji, 2015-06-30 18:11:05]

I agree, having a function that doesn't close over its lexical environment is
weird. But having to write the Worker as a string is pretty horrible.

Perhaps you're right that it's better to make the stringification of the
function explicit. I'm happy to do that in the next CL if everyone prefers it.

https://codereview.chromium.org/1216023003/diff/20001/test/mjsunit/d8-worker-...
File test/mjsunit/d8-worker-sharedarraybuffer.js (right):

https://codereview.chromium.org/1216023003/diff/20001/test/mjsunit/d8-worker-...
test/mjsunit/d8-worker-sharedarraybuffer.js:31: function f() {
On 2015/06/30 at 17:42:43, jarin wrote:
> I do not think this is doing what you think it's doing. E.g.
> 
> var x = false;
> if (x) function f() { print("f"); }
> f();
> 
> still happily prints f.
> 
> You really have to say 
> var f = function () { ... }
> 
> Even then I am not entirely sure why this has to be conditioned on this.Worker
being defined?

Oh, I wasn't trying to prevent the definition of f, it's just that sometimes
Worker isn't defined, so I was wrapping the whole test in the Worker check. I'll
move f out to make that clearer.

https://codereview.chromium.org/1216023003/diff/20001/test/mjsunit/d8-worker-...
test/mjsunit/d8-worker-sharedarraybuffer.js:36: throw new
Error("SharedArrayBuffer transfer byteLength");
On 2015/06/30 at 17:42:43, jarin wrote:
> Nit: Could we get the braces here and below? (In general, we like braces if
the body is not on the same line.)

Done.

https://codereview.chromium.org/1216023003/diff/20001/test/mjsunit/d8-worker-...
test/mjsunit/d8-worker-sharedarraybuffer.js:55: assertEquals(16,
sab.byteLength);  // ArrayBuffer should not neutered.
On 2015/06/30 at 17:42:43, jarin wrote:
> In the comment: ... should not *be* neutered.

Done.

https://codereview.chromium.org/1216023003/diff/20001/test/mjsunit/d8-worker-...
test/mjsunit/d8-worker-sharedarraybuffer.js:63: w.terminate();
On 2015/06/30 at 17:42:43, jarin wrote:
> Could we also check that the array has not been neutered after termination?

Done.

[Jarin, 2015-06-30 18:21:29]

On 2015/06/30 18:11:05, binji wrote:
> On 2015/06/30 at 17:42:43, jarin wrote:
> > I do not think this is doing what you think it's doing. E.g.
> > 
> > var x = false;
> > if (x) function f() { print("f"); }
> > f();
> > 
> > still happily prints f.
> > 
> > You really have to say 
> > var f = function () { ... }
> > 
> > Even then I am not entirely sure why this has to be conditioned on
this.Worker
> being defined?
> 
> Oh, I wasn't trying to prevent the definition of f, it's just that sometimes
> Worker isn't defined, so I was wrapping the whole test in the Worker check.
I'll
> move f out to make that clearer.

You are right, I indeed somehow misread the code. Please, ignore this comment.

[Jarin, 2015-06-30 18:30:24]

On 2015/06/30 18:11:05, binji wrote:
> I agree, having a function that doesn't close over its lexical environment is
> weird. But having to write the Worker as a string is pretty horrible.

(You'd be surprised how much we already use code in strings for eval for various
reasons in our test suite, so this would not be introducing particularly new
kind of horribleness.)

> Perhaps you're right that it's better to make the stringification of the
> function explicit. I'm happy to do that in the next CL if everyone prefers it.

Right, I will ask around tomorrow. I only talked to Benedikt and he also thought
the current way is confusing (and less consistent with the standard web
workers).

In any case, this CL still LGTM. I totally agree that if we go ahead with the
Worker(<string>) constructor, it should be a separate CL.

Thanks!

[binji, 2015-06-30 20:20:09]

The CQ bit was checked by binji@chromium.org

[commit-bot: I haz the power, 2015-06-30 20:20:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1216023003/40001

[commit-bot: I haz the power, 2015-06-30 20:22:45]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-30 20:23:00]

Patchset 3 (id:??) landed as
https://crrev.com/686e675734579e393fa813c745716584a90e9e49
Cr-Commit-Position: refs/heads/master@{#29394}