Closed Bug 820875 Opened 7 years ago Closed 7 years ago

Implement MutexBase on Windows

Categories

(Core :: General, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #691365 - Flags: review?(netzen)
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)
Attachment #691365 - Flags: review?(netzen)
OS: Mac OS X → Windows 7
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.
Thanks, Ehsan!
https://hg.mozilla.org/mozilla-central/rev/caf88c568805
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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!)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Patch (v2)Splinter Review
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!
Attachment #691365 - Attachment is obsolete: true
Attachment #692086 - Flags: review?(netzen)
Have you verified maybe by profiling the creation/destruction of an object in a large loop?
This is the part I was referring to by the way: "less expensive"
I'm not sure if I understand what you mean.

FWIW I have not _yet_ built this code and played with it.
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.
Attachment #692086 - Flags: review?(netzen) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/6fbb674858da
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Comment on attachment 692086 [details] [diff] [review]
Patch (v2)

[Triage Comment]
npotb
Attachment #692086 - Flags: approval-mozilla-b2g18+
Attachment #692086 - Flags: approval-mozilla-aurora+
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.
(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!
You need to log in before you can comment on or make changes to this bug.