If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add a `Mutex<T>` type, based on Rust's `std::sync::Mutex<T>`

RESOLVED FIXED in Firefox 47

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
Created attachment 8724147 [details] [diff] [review]
Add a `RwLock<T>` type, based on Rust's `std::sync::RwLock<T>`

Still some open questions, as marked by "TODO FITZGEN", but I figured I'd get
some feedback now since it is pretty much done.
Attachment #8724147 - Flags: feedback?(terrence)
Attachment #8724147 - Flags: feedback?(jimb)
Also, this seems like the kind of thing that would ideally be in mfbt or js/public, but I don't think we can use NSPR in those places?
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED

Comment 3

2 years ago
Comment on attachment 8724147 [details] [diff] [review]
Add a `RwLock<T>` type, based on Rust's `std::sync::RwLock<T>`

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

::: js/src/jsapi-tests/testRwLock.cpp
@@ +25,5 @@
> +
> +    js::RwLock<Counter> counter(mozilla::Move(*maybeCounter));
> +
> +    // TODO FITZGEN: actually, ya know, spawn threads that touch the counter
> +    // concurrently.

These kinds of concurrency tests often turn into "burn-in" tests where you want to let them run as long as possible. But that's not really what jsapi-tests should be. I suggest writing a test that runs until each thread has *seen* a change by another thread, and then exiting immediately. This will ensure that:

- we've actually seen some interaction (as opposed to each thread running its bit as soon as it's spawned and exiting before the next thread even gets started)

- we actually generate some real contention (as opposed to tests that do random sleeps to ensure shuffling, but end up never letting two threads run at the same time because the delays are so much larger than the run times),

- and yet the test still runs quickly.

::: js/src/vm/RwLock.cpp
@@ +51,5 @@
> +
> +    MOZ_ASSERT(readersCount_ > 0);
> +    readersCount_--;
> +    if (readersCount_ == 0)
> +        MOZ_RELEASE_ASSERT(PR_Unlock(globalLock_) == PR_SUCCESS);

I don't think this works. According to the docs for PR_Unlock, it can only be called by the same thread that called PR_Lock on the lock. In a rwlock, there's no reason to believe that the reader unlocking globalLock_ would be the same thread that originally locked it.

http://www-archive.mozilla.org/projects/nspr/reference/html/prlock.html#16066

Also, doesn't this let readers starve writers? That is, if there's a continuous stream of overlapping readers, there's no reason they would ever notice someone waiting on globalLock_.

::: js/src/vm/RwLock.h
@@ +153,5 @@
> +     * On success, `mozilla::Some` is returned. On failure, `mozilla::Nothing`
> +     * is returned.
> +     */
> +    template <typename U>
> +    static mozilla::Maybe<RwLock<T>> Create(U&& u) {

I really think we should just expose a constructor, as C++ does, rather than emulating Rust's practice of returning a new instance by value. It's just too distracting.
Attachment #8724147 - Flags: feedback?(jimb) → feedback-
(In reply to Jim Blandy :jimb from comment #3)
> I don't think this works. According to the docs for PR_Unlock, it can only
> be called by the same thread that called PR_Lock on the lock.

Yikes, I missed that :(

I think this invalidates the whole approach. I guess I don't *need* multiple readers and single writer for what I was going to use this for, it was just a nice-to-have and I can use a mutex instead, which wouldn't have this unlock-on-a-different thread issue.

> I really think we should just expose a constructor, as C++ does, rather than
> emulating Rust's practice of returning a new instance by value. It's just
> too distracting.

Because construction needs to be infallible, we would need to split the type into two states internally: initialized and not initialized. Then every method would have to start asserting that the thing was initialized. I don't think that is any less distracting, but I will defer to you if you feel strongly about it.

Comment 5

2 years ago
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #4)
> Because construction needs to be infallible, we would need to split the type
> into two states internally: initialized and not initialized. Then every
> method would have to start asserting that the thing was initialized. I don't
> think that is any less distracting, but I will defer to you if you feel
> strongly about it.

Oh, that's a good point. I had skimmed over the Maybe in the return type. Put that way, a Maybe<Self> create method makes a lot of sense.
Created attachment 8724267 [details] [diff] [review]
Add a `Mutex<T>` type, based on Rust's `std::sync::Mutex<T>`
Attachment #8724267 - Flags: review?(terrence)
Attachment #8724267 - Flags: review?(jimb)
Attachment #8724147 - Attachment is obsolete: true
Attachment #8724147 - Flags: feedback?(terrence)
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b999009c6787
Summary: Add a `RwLock<T>` type, based on Rust's `std::sync::RwLock<T>` → Add a `Mutex<T>` type, based on Rust's `std::sync::Mutex<T>`
Comment on attachment 8724267 [details] [diff] [review]
Add a `Mutex<T>` type, based on Rust's `std::sync::Mutex<T>`

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

::: js/src/vm/Mutex.h
@@ +35,5 @@
> +
> +    ~MutexBase();
> +
> +  protected:
> +    MutexBase(PRLock* lock)

This needs explicit, as the try push says.

Comment 9

2 years ago
Comment on attachment 8724267 [details] [diff] [review]
Add a `Mutex<T>` type, based on Rust's `std::sync::Mutex<T>`

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

::: js/src/vm/Mutex.h
@@ +96,5 @@
> + *         Mutex<Counter> counter;
> + *
> + *       public:
> + *         void inc(size_t n) {
> + *             auto guard = counter.lock();

What is the advantage of returning a guard by value, instead of making it an ordinary RAII class, and writing this instead?

    MutexGuard guard(counter);
    guard->inc(n);

I think that's what C++ programmers would expect.

Updated

2 years ago
Flags: needinfo?(nfitzgerald)
(In reply to Jim Blandy :jimb from comment #9)
> What is the advantage of returning a guard by value, instead of making it an
> ordinary RAII class, and writing this instead?
> 
>     MutexGuard guard(counter);
>     guard->inc(n);
> 
> I think that's what C++ programmers would expect.

Note that you *can* do that now with minor changes:

  Mutex<Counter>::Guard guard(counter);
  guard->inc(n);

However, you have the choice to do either. I can, of course, get rid of the `lock()` method if you don't think it carries its weight. I mildly prefer the `lock()` method and its relative concision, but not enough to argue about it.
Flags: needinfo?(nfitzgerald)

Comment 11

2 years ago
Comment on attachment 8724267 [details] [diff] [review]
Add a `Mutex<T>` type, based on Rust's `std::sync::Mutex<T>`

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

::: js/src/vm/Mutex.h
@@ +70,5 @@
> + *       public:
> + *         void inc(int32_t n) { i += n; }
> + *     };
> + *
> + * If we wanted to share a counter across threads with `std::mutex`, we rely

"wanted" and "rely" should agree on tense:

If share a counter across threads with `std::mutex`, we rely ...
Attachment #8724267 - Flags: review?(jimb) → review+
Comment on attachment 8724267 [details] [diff] [review]
Add a `Mutex<T>` type, based on Rust's `std::sync::Mutex<T>`

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

Nice!

::: js/src/jsapi-tests/testMutex.cpp
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "jsapi-tests/tests.h"
> +
> +#include "vm/Mutex.h"

This is missing imports for Vector and probably others. Please try building with FILES_PER_UNIFIED_FILE=1 to make sure you got everything.

@@ +20,5 @@
> +        MOZ_ASSERT(bit < 32);
> +    }
> +};
> +
> +void printDiagnosticMessage(uint32_t seen) {

SM style calls for 3 lines here:

void
printDiagnosticMessage(uint32_t seen)
{

@@ +31,5 @@
> +    }
> +    fprintf(stderr, "\n");
> +}
> +
> +void setBitAndCheck(void* arg) {

And here.

@@ +65,5 @@
> +
> +    js::Vector<PRThread*> threads(cx);
> +    CHECK(threads.reserve(32));
> +
> +    for (uint8_t i = 0; i < 32; i++) {

Please put |const static uint8_t numThreads = 32;| at the top and use it instead of 32 to make sure these don't get out of sync. Also, optionally |for (auto i : mozilla::MakeRange(numThreads)) {|.

::: js/src/vm/Mutex.h
@@ +17,5 @@
> +namespace detail {
> +
> +class MutexBase
> +{
> +  private:

|class| is private by default, so you can elide this.

@@ +25,5 @@
> +    MutexBase& operator=(const MutexBase&) = delete;
> +
> +  public:
> +    // This move constructor is only public for `mozilla::Forward`.
> +    MutexBase(MutexBase&& rhs)

And I think you need explicit here too.

@@ +26,5 @@
> +
> +  public:
> +    // This move constructor is only public for `mozilla::Forward`.
> +    MutexBase(MutexBase&& rhs)
> +        : lock_(rhs.lock_)

2 spaces before the :.

@@ +167,5 @@
> +        Guard& operator=(const Guard&) = delete;
> +
> +      public:
> +        explicit Guard(const Mutex& parent)
> +            : parent_(&parent)

2 spaces.

@@ +191,5 @@
> +            return parent_->value_;
> +        }
> +
> +        operator T& () { return get(); }
> +        T* operator->() { return &get(); }

These two should also be const, I think.
Attachment #8724267 - Flags: review?(terrence) → review+
Depends on: 1252713
Created attachment 8725500 [details] [diff] [review]
Add a `Mutex<T>` type, based on Rust's `std::sync::Mutex<T>`
Attachment #8725500 - Flags: review+
Attachment #8724267 - Attachment is obsolete: true
No longer depends on: 1252713
Depends on: 1252713
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=380309500c25
Created attachment 8725742 [details] [diff] [review]
Add a `Mutex<T>` type, based on Rust's `std::sync::Mutex<T>`

Fix a bad header include order.
Attachment #8725742 - Flags: review+
Attachment #8725500 - Attachment is obsolete: true
New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=382b86194129
Created attachment 8725813 [details] [diff] [review]
Add a `Mutex<T>` type, based on Rust's `std::sync::Mutex<T>`

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=db9db2278857
Attachment #8725813 - Flags: review+
Attachment #8725742 - Attachment is obsolete: true
Note that bug 1252713, which this depends on, is also marked checkin-needed at the moment.
Keywords: checkin-needed

Comment 19

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5d666bda9a7
Keywords: checkin-needed

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f5d666bda9a7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.