1333429 - make js::Mutex unmovable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nathan Froyd [:froydnj]]

Bug 1257019 made js::Mutex move-constructible.  It did this by dynamically allocating the platform-specific mutex information inside Mutex's constructor, e.g.:

https://hg.mozilla.org/mozilla-central/rev/d5e77c8d64c9#l3.21

In the specific case of pthreads (not sure about Windows, but would not be terribly surprised to learn the situation is similar), copying pthread_mutex_t is not allowed.

While doing this deleted some grotty code:

https://hg.mozilla.org/mozilla-central/rev/d5e77c8d64c9#l2.53

it also made moving the mutex implementations out of the JS engine difficult, as the guts of Mutex now depends on JS-specific things like AutoEnterOOMUnsafeRegion.  Additionally, every use of Mutex now pays some small cost of dynamic allocation that was previously avoided, rather than only those consumers who need move construction.  I note also that std::mutex/std::recursive_mutex are defined as having deleted copy/assignment operations, while not having defined move/assignment operations, which means those types are not movable.

Therefore, I propose that we should revert parts of bug 1257019 (this is probably a little more complicated than a straight backout, since bug 1309909 intervened and shuffled things around).  Any consumer that needs movable mutexes can have UniquePtr<Mutex> members.

I thought this would be somewhat complicated, but #ifdef'ing out the move constructor/assignment operator and the associated tests shows no compile failures on x86-64 Linux.  Bug 1257019 was landed in service of bug 1211723, but it seems that the patches in bug 1211723 didn't make any use of Mutex's movability...or things have changed a bit in the last several months.

fitzgen, thoughts?

Comment 1

[Jeff Walden [:Waldo]]

Driveby concur in removing what seemed like a fugly hack at the time it landed.

Comment 2

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

It was indeed a fugly hack, and if we can successfully compile without the move constructors now, then we should kill them.

> Any consumer that needs movable mutexes can have UniquePtr<Mutex> members.

Yep.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 1 - remove move construction/assignment from js::Mutex

We do this for several reasons:

- Making Mutex movable requires a separate memory allocation for every
  Mutex, but not every Mutex client wants a movable Mutex.  If
  movability is desired, a client can use UniquePtr<Mutex>.

- Inserting movability here requires certain JS engine-specific
  constructs (e.g. OOM checking) that will be inconvenient if Mutexes
  are lifted out of the JS engine.

- The related class ConditionVariable isn't movable; the related classes
  std::mutex and std::recursive_mutex aren't movable either.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 2 - don't dynamically allocate platform data in js::Mutex

Dynamically allocating platform data was necessary when movability was
supported, as mutexes on certain platforms (e.g. pthread_mutex_t) cannot
be copied.  But now that movability is no longer supported, we can go
back to having the platform data be a normal member of the Mutex object.

This compiles locally; running it through try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8af3d0cec4a8fd888fb7492f3638e036daa525b

Ideally I got the Windows parts correct.

Comment 5

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

Comment on attachment 8830001 [details] [diff] [review]
part 1 - remove move construction/assignment from js::Mutex

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

\o/

Comment 6

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

Comment on attachment 8830002 [details] [diff] [review]
part 2 - don't dynamically allocate platform data in js::Mutex

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

Great!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/567ea9cd0450
part 1 - remove move construction/assignment from js::Mutex; r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8bb22053e65
part 2 - don't dynamically allocate platform data in js::Mutex; r=fitzgen

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/567ea9cd0450
https://hg.mozilla.org/mozilla-central/rev/e8bb22053e65