Closed Bug 1280104 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Iteration:
50.2 - Jul 4
Tracking Status
firefox50 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b6d112b3c92
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Attachment #8762739 - Attachment is obsolete: true
Attachment #8762739 - Flags: review?(terrence)
Blocks: 1263289
Iteration: --- → 50.1
Flags: qe-verify-
Priority: -- → P1
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/
Attachment #8762740 - Flags: review?(terrence) → review+
Iteration: 50.1 → 50.2
Whiteboard: [devtools-html]
Blocks: 956899
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_` ?
> 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.
Pushed by nfitzgerald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9338313016a
Use js::Mutex/js::ConditionVariable instead of PRLock/PRCondVar in js::Monitor; r=terrence
https://hg.mozilla.org/mozilla-central/rev/c9338313016a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Whiteboard: [devtools-html]
You need to log in before you can comment on or make changes to this bug.