1280104 - Use js::Mutex/js::ConditionVariable instead of PRLock/PRCondVar in js::Monitor

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Use js::Mutex/js::ConditionVariable instead of PRLock/PRCondVar in js::Monitor

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b6d112b3c92

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Use js::Mutex/js::ConditionVariable instead of PRLock/PRCondVar in js::Monitor

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9701df4719b5

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 8762740 [details] [diff] [review]
Use js::Mutex/js::ConditionVariable instead of PRLock/PRCondVar in js::Monitor

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

::: js/src/vm/Monitor.cpp
@@ -19,5 @@
> -    if (!condVar_)
> -        return false;
> -
> -    return true;
> -}

\o/

Comment 6

[Bert Belder]

Comment on attachment 8762740 [details] [diff] [review]
Use js::Mutex/js::ConditionVariable instead of PRLock/PRCondVar in js::Monitor

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

::: js/src/threading/Mutex.h
@@ +40,5 @@
>      return *this;
>    }
>  
> +  bool operator==(const Mutex& rhs) {
> +    return platformData_ == rhs.platformData_;

This will probably work in practice, but it doesn't seem correct to me; there's no guarantee that the *contents* of platformData_ uniquely identifies the lock (consider an implementation that uses a spinlock - platformData_ may just be counter).

Wouldn't it be more obvious to do `return this == &rhs` or, if you insist, `return &platformData_ == &rhs.platformData_` ?

Comment 7

[Bert Belder]

> This will probably work in practice, but it doesn't seem correct to me; there's no guarantee that the *contents* of platformData_ uniquely identifies the lock (consider an implementation that uses a spinlock - platformData_ may just be counter).

I was being dumb here - platformData_ is an array, so it already compares pointers and not contents.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9338313016a
Use js::Mutex/js::ConditionVariable instead of PRLock/PRCondVar in js::Monitor; r=terrence

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c9338313016a