Closed Bug 1411084 Opened 7 years ago Closed 7 years ago

Use RAII for mutex locking

Categories

(Core :: Memory Allocator, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file)

      No description provided.
Assignee: nobody → mh+mozilla
Comment on attachment 8921265 [details]
Bug 1411084 - Use RAII for mutex locking in mozjemalloc.

https://reviewboard.mozilla.org/r/192278/#review197450

RAII is good. r=me with the MOZ_GUARD_OBJECT stuff added, as per below.

::: memory/build/mozjemalloc.cpp:527
(Diff revision 1)
>    inline void Unlock();
>  };
>  
> +struct MOZ_RAII MutexAutoLock
> +{
> +  explicit MutexAutoLock(Mutex& aMutex) : mMutex(aMutex) { mMutex.Lock(); }

Please add the MOZ_GUARD_OBJECT stuff so this can't be mis-constructed.

::: memory/build/mozjemalloc.cpp:1749
(Diff revision 1)
>  
>    if (slot) {
>      ret = *slot;
>    }
>  #ifdef MOZ_DEBUG
> -  mlock.Lock();
> +  MutexAutoLock lock(mlock);

What/where is `mlock`? I can't find it...
Attachment #8921265 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #2)
> ::: memory/build/mozjemalloc.cpp:1749
> (Diff revision 1)
> >  
> >    if (slot) {
> >      ret = *slot;
> >    }
> >  #ifdef MOZ_DEBUG
> > -  mlock.Lock();
> > +  MutexAutoLock lock(mlock);
> 
> What/where is `mlock`? I can't find it...

Argh, it's a typo that comes from bug 1406303, and that went undetected because as of bug 1365460, debug stuff is gated on MOZ_DEBUG... which is actually never defined. The actual debug define is DEBUG (which, I agree, is confusing). I'll file a couple followups for that.
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/25bc10affd99
Use RAII for mutex locking in mozjemalloc. r=njn
https://hg.mozilla.org/mozilla-central/rev/25bc10affd99
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: