Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 5 months ago by binji Modified:
5 years, 5 months ago Reviewers:
Jarin CC:
Benedikt Meurer, Michael Starzinger, v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Descriptiond8 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}
Patch Set 1 #Patch Set 2 : check value after spinwait #
Total comments: 9
Patch Set 3 : feedback #
Created: 5 years, 5 months ago
Messages
Total messages: 9 (1 generated)
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?
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.
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.
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!
The CQ bit was checked by binji@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1216023003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/686e675734579e393fa813c745716584a90e9e49 Cr-Commit-Position: refs/heads/master@{#29394} |
||||||||||||||||||||||||||||||||||||||||||||||||||
