Bug 167431 – jsc.cpp should have the $.agent stuff for testing SAB

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming

[Filip Pizlo]

Created attachment 299737 [details]
work in progress

[Filip Pizlo]

Created attachment 299758 [details]
the patch

[WebKit Commit Bot]

Attachment 299758 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jsc.cpp:2488:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 299758 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299758&action=review

r=me

> Source/JavaScriptCore/jsc.cpp:2348
> +    if (isOnList())

Why would this not be true?

> Source/JavaScriptCore/jsc.cpp:2396
> +    for (Worker* worker = m_workers.begin(); worker != m_workers.end(); worker = worker->next())
> +        func(locker, *worker);

Should we ignore Worker::currentWorker()?

> Source/JavaScriptCore/jsc.cpp:2478
> +    Lock didStartLock;
> +    Condition didStartCondition;
> +    bool didStart = false;

Nit: Why not just do this using a single lock that starts off locked and is unlocked once runJSC calls the lambda?
Then the code below could just do:
didStartLock.lock().

> Source/JavaScriptCore/jsc.cpp:2559
> +        sleep(Seconds::fromMilliseconds(exec->argument(0).toNumber(exec)));

Might want to exception check toNumber?

> Source/JavaScriptCore/jsc.cpp:2579
> +            nativeBuffer->transferTo(contents); // "transferTo" means "share" if the buffer is shared.

What if it's not a SAB? Will we just give our contents to the first worker on the list? What if it's ourself?

> Source/JavaScriptCore/jsc.cpp:2581
> +            worker.send(locker, message);

the name of the send function kind of confuses me because I always read it as "the worker will send something to something else", but really, I think it should be read as, the currentWorker() is sending something to all the individual workers. Not sure I have better ideas for a name, but maybe you do? Maybe it's just me who reads it this way.

[Filip Pizlo]

(In reply to comment #4)
> Comment on attachment 299758 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299758&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/jsc.cpp:2348
> > +    if (isOnList())
> 
> Why would this not be true?

I changed it to assert that it's true.

> 
> > Source/JavaScriptCore/jsc.cpp:2396
> > +    for (Worker* worker = m_workers.begin(); worker != m_workers.end(); worker = worker->next())
> > +        func(locker, *worker);
> 
> Should we ignore Worker::currentWorker()?

Makes sense. I made it ignore &Worker::current().

> 
> > Source/JavaScriptCore/jsc.cpp:2478
> > +    Lock didStartLock;
> > +    Condition didStartCondition;
> > +    bool didStart = false;
> 
> Nit: Why not just do this using a single lock that starts off locked and is
> unlocked once runJSC calls the lambda?
> Then the code below could just do:
> didStartLock.lock().

Not sure what you're suggesting. Seems like you mean that we lock the lock before starting the thread and unlock the lock inside the thread.

Note that in the literature, the "lock" and "mutex" ADTs are thought to have this rule: "unlock() must be called from the same thread that called lock()". If you want a thing that's like a lock but unlock() can be called from a different thread, then you want a "binary semaphore". I've also heard this called a latch, and Win32 calls it an auto-reset event. If you really want, I could use the BinarySemaphore class in WTF. I have a habit of just using Lock+Condition because I enjoy the uniformity of using it everywhere.

It so happens that our lock implementation probably works when unlock() is called from a different thread than lock(), but this isn't something we should rely on, because:

- Someone attempting alternate implementations of Lock could reasonably assume that unlock() is always called from the same thread as lock().

- Several lock implementations that I think we want to consider using in WebKit rely on the lock/unlock symmetry. Lock implementations that automatically provide deadlock diagnostics rely on this. Lock implementations that automatically resolve priority inversion rely on this.

> 
> > Source/JavaScriptCore/jsc.cpp:2559
> > +        sleep(Seconds::fromMilliseconds(exec->argument(0).toNumber(exec)));
> 
> Might want to exception check toNumber?

Oh, lol. Fixed.

> 
> > Source/JavaScriptCore/jsc.cpp:2579
> > +            nativeBuffer->transferTo(contents); // "transferTo" means "share" if the buffer is shared.
> 
> What if it's not a SAB? Will we just give our contents to the first worker
> on the list? What if it's ourself?

This code is guarded by:

    if (!jsBuffer || !jsBuffer->isShared())
        return JSValue::encode(throwException(exec, scope, createError(exec, ASCIILiteral("Expected SharedArrayBuffer"))));

> 
> > Source/JavaScriptCore/jsc.cpp:2581
> > +            worker.send(locker, message);
> 
> the name of the send function kind of confuses me because I always read it
> as "the worker will send something to something else", but really, I think
> it should be read as, the currentWorker() is sending something to all the
> individual workers. Not sure I have better ideas for a name, but maybe you
> do? Maybe it's just me who reads it this way.

I changed Worker::send() and Worker::receive() to Worker::enqueue() and Worker::dequeue().

[Saam Barati]

Comment on attachment 299758 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299758&action=review

>>> Source/JavaScriptCore/jsc.cpp:2478
>>> +    bool didStart = false;
>> 
>> Nit: Why not just do this using a single lock that starts off locked and is unlocked once runJSC calls the lambda?
>> Then the code below could just do:
>> didStartLock.lock().
> 
> Not sure what you're suggesting. Seems like you mean that we lock the lock before starting the thread and unlock the lock inside the thread.
> 
> Note that in the literature, the "lock" and "mutex" ADTs are thought to have this rule: "unlock() must be called from the same thread that called lock()". If you want a thing that's like a lock but unlock() can be called from a different thread, then you want a "binary semaphore". I've also heard this called a latch, and Win32 calls it an auto-reset event. If you really want, I could use the BinarySemaphore class in WTF. I have a habit of just using Lock+Condition because I enjoy the uniformity of using it everywhere.
> 
> It so happens that our lock implementation probably works when unlock() is called from a different thread than lock(), but this isn't something we should rely on, because:
> 
> - Someone attempting alternate implementations of Lock could reasonably assume that unlock() is always called from the same thread as lock().
> 
> - Several lock implementations that I think we want to consider using in WebKit rely on the lock/unlock symmetry. Lock implementations that automatically provide deadlock diagnostics rely on this. Lock implementations that automatically resolve priority inversion rely on this.

Yeah I thought about this before posting my comment, but thought to post it anyways. I agree we should stick with the code you have now.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/211194

[Radar WebKit Bug Importer]

<rdar://problem/30201008>