820875 - Implement MutexBase on Windows

Status: RESOLVED FIXED

(Core :: General, defect)

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Brian R. Bondy [:bbondy]]

Comment on attachment 691365 [details] [diff] [review]
Patch (v1)

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

Would be good to have some way to know if the Mutex is working or not.  Or perhaps at least a log warning.
r=me with those changes or re-request review if you disagree with any of the below with explanation.

::: memory/replace/dmd/DMD.cpp
@@ +270,5 @@
> +
> +public:
> +  MutexBase()
> +    : mMutex(CreateMutexW(nullptr, false, nullptr))
> +  {}

Based on the name of the class I think you may be implementing derived classes.  If so please make a virtual destructor.

Also the destructor should call if (mMutex) CloseHandle(mMutex) or else there will be a HANDLE leak which may eventually lead to this returning NULL for everything.

@@ +273,5 @@
> +    : mMutex(CreateMutexW(nullptr, false, nullptr))
> +  {}
> +
> +  void Lock()
> +  {

if (mMutex)

@@ +278,5 @@
> +    WaitForSingleObject(mMutex, INFINITE);
> +  }
> +
> +  void Unlock()
> +  {

if (mMutex)

Comment 3

[(no longer active)]

Comment on attachment 691365 [details] [diff] [review]
Patch (v1)

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

I think I'll MOZ_ASSERT this...  Not sure if the null checks will make much sense but I'll add them anyway.

::: memory/replace/dmd/DMD.cpp
@@ +270,5 @@
> +
> +public:
> +  MutexBase()
> +    : mMutex(CreateMutexW(nullptr, false, nullptr))
> +  {}

The virtual dtor is not necessary here since we don't delete these objects through base class references, but CloseHandle is definitely necessary, and it's sort of embarrassing I missed it.

Comment 4

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/caf88c568805

Comment 5

[Nicholas Nethercote [inactive]]

Thanks, Ehsan!

Comment 6

[Ed Morley [:emorley]]

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

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 691365 [details] [diff] [review]
Patch (v1)

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

Oh:  DMD takes a heavy abort-if-anything-OOMs approach.  So the MOZ_ASSERT(mMutex) should be ExitOnFailure(mMutex), and then the |if (mMutex)| conditions can be removed.  (In fact, the current code is pretty dubious;  if the mutex creation fails all the Lock/Unlock calls will be silent no-ops!)

Comment 8

[(no longer active)]

Attachment: Patch (v2)

Critical sections are much less expensive, and allow us to get away without doing any error checking whatsoever.  Two nice points over the mutex based approach!

Comment 9

[Brian R. Bondy [:bbondy]]

Have you verified maybe by profiling the creation/destruction of an object in a large loop?

Comment 10

[Brian R. Bondy [:bbondy]]

This is the part I was referring to by the way: "less expensive"

Comment 11

[(no longer active)]

I'm not sure if I understand what you mean.

FWIW I have not _yet_ built this code and played with it.

Comment 12

[Brian R. Bondy [:bbondy]]

I just meant have you tested performance wise out of curiosity. I know critical sections are said to be more lightweight though so that's fine that you haven't.

Comment 13

[(no longer active)]

Oh I see.  Critical sections are basically user mode spin locks, so they won't be mapped to a kernel system call, which is why they're considerably faster.

Comment 14

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fbb674858da

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6fbb674858da

Comment 16

[Justin Lebar (not reading bugmail)]

Comment on attachment 692086 [details] [diff] [review]
Patch (v2)

[Triage Comment]
npotb

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/942c7448b0f4
https://hg.mozilla.org/releases/mozilla-aurora/rev/87986d0f5d05

https://hg.mozilla.org/releases/mozilla-b2g18/rev/2c10882f7f3d
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ae183d4756e0

Comment 18

[Laurențiu Nicola]

Actually, slim reader/writer locks are even faster where they are available (see http://nasutechtips.blogspot.ro/2010/11/slim-read-write-srw-locks.html , for example) and don't need the destructor.

Comment 19

[(no longer active)]

(In reply to comment #18)
> Actually, slim reader/writer locks are even faster where they are available
> (see http://nasutechtips.blogspot.ro/2010/11/slim-read-write-srw-locks.html ,
> for example) and don't need the destructor.

And they're not supported on Windows XP, so we can't use them!