Closed Bug 1385335 Opened 3 years ago Closed 3 years ago

Use RecursiveMutex in AsyncPanZoomController

Categories

(Core :: Panning and Zooming, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fix-optional
firefox58 --- fixed

People

(Reporter: botond, Assigned: kausam2015, Mentored)

References

Details

(Whiteboard: [gfx-noted] [lang=c++])

Attachments

(1 file, 3 obsolete files)

AsyncPanZoomController uses ReentrantMonitor, I believe solely for its recursive locking capabilities.

Now that we have a lighter-weight facility for that purpose, RecursiveMutex, we should try using that instead.
This might make a nice mentored bug.
Mentor: botond
Whiteboard: [gfx-noted] → [gfx-noted] [lang=c++]
I want to work on this issue.
(In reply to Cosm from comment #2)
> I want to work on this issue.

Sure!

The first step is to build Firefox using these instructions [1], if you haven't already.

Please post back once you've done that, and I'll give some further guidance. Thanks!

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
Thanks Botond.
For the bug, I found AsyncPanZoomController.cpp which is using ReentrantMonitorAutoEnter lock(...) function. We want to replace all the calls to this function with RecursiveMutexAutoLock lock(...). Is that right?
And can you please explain how is the RecursiveMutex lighter than ReentrantMonitor ?
(In reply to Cosm from comment #4)
> For the bug, I found AsyncPanZoomController.cpp which is using
> ReentrantMonitorAutoEnter lock(...) function. 

Small clarification: 'lock' here is not a function, it's a variable. The variable will lock ("enter") the monitor passed as an argument in its constructor, and unlock ("exit") it in its destructor, so that it remains locked while the variable is in scope.

> We want to replace all the
> calls to this function with RecursiveMutexAutoLock lock(...). Is that right?

Yep.

We'll also want to replace the monitor itself (AsyncPanZoomController::mMonitor [1]) with a RecursiveMutex.

> And can you please explain how is the RecursiveMutex lighter than
> ReentrantMonitor ?

Nathan Froyd talks a bit about this in the bug that introduced RecursiveMutex [2]:

"ReentrantMonitor is more expensive than simple recursive mutexes because of its need to support notifications. 
...
Tiny benchmarks on my Linux machine suggest that a lock/unlock pair for true recursive mutexes is 3-5x faster than enter/exit for ReentrantMonitor."

[1] http://searchfox.org/mozilla-central/rev/d29c832536839b60a9ee5b512eeb934274b879b0/gfx/layers/apz/src/AsyncPanZoomController.h#740
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1347963#c0
Attached patch patchf.patch (obsolete) — Splinter Review
Attachment #8906548 - Flags: review?(botond)
Cosm, did you build Firefox after writing this patch? I tried building it locally and got a number of compiler errors.
Comment on attachment 8906548 [details] [diff] [review]
patchf.patch

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

Clearing review flag until compiler errors are fixed.
Attachment #8906548 - Flags: review?(botond)
(Assigning the bug to you to reflect that you're working on it.)
Assignee: nobody → kausam2015
Attached patch patchm.patch (obsolete) — Splinter Review
The patch when applied, compiles and runs successfully now. 
But I do not know how to measure performance improvements caused by this patch. 
Please let me know if any further edits are needed.
Attachment #8908282 - Flags: review?(botond)
Attachment #8906548 - Attachment is obsolete: true
I still get compiler errors in a debug build:

/home/botond/dev/mozilla/central/objdir-desktop-clang/dist/include/mozilla/RecursiveMutex.h:43:41: error: use of undeclared identifier 'mRecursiveMutex'
    PR_ASSERT_CURRENT_THREAD_IN_MONITOR(mRecursiveMutex);
                                        ^
Comment on attachment 8908282 [details] [diff] [review]
patchm.patch

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

Thanks - apart from the changes in RecursiveMutex.h which I commented on below, this patch looks good! Clearing review flag until that is resolved.

I have just one more request: please update the reference to AsyncPanZoomController::mMonitor in this comment:

  http://searchfox.org/mozilla-central/rev/2c9a5993ac40ec1db8450e3e0a85702fa291b9e2/gfx/layers/apz/src/APZCTreeManager.h#57

to mRecursiveMutex.

::: xpcom/threads/RecursiveMutex.h
@@ +39,5 @@
> +   * @see prmon.h
> +   **/
> +  void AssertCurrentThreadIn()
> +  {
> +    PR_ASSERT_CURRENT_THREAD_IN_MONITOR(mRecursiveMutex);

So this line is causing a compiler error, because you're trying to implement RecursiveMutex::AssertCurrentThreadIn(), but this implementation is incorrect (it references a member variable named "mRecursiveMutex" which doesn't exist).

However, there is no need to implement RecursiveMutex::AssertCurrentThreadIn(), because it's already implemented in BlockingResourceBase.cpp [1], so here we can keep it as just a declaration.

Adding AssertNotCurrentThreadIn() as you did is fine. I would remove the doc-comments though (we don't want to point readers to "prmon.h" here, because that's a header specific to monitors), and in the (empty) body of the DEBUG version I would add a comment like:

  // Not currently implemented. See bug 476536 for discussion.

(since bug 476536 contains discussion about how we would implement this function if we want to do so at a later time).

[1] http://searchfox.org/mozilla-central/rev/2c9a5993ac40ec1db8450e3e0a85702fa291b9e2/xpcom/threads/BlockingResourceBase.cpp#586
Attachment #8908282 - Flags: review?(botond)
(By the way, I saw you messaged me on IRC. Sorry for missing you. For a better chance of reaching me, ping me (meaning, mention my name) in #developers or #apz, rather than sending a direct message. You may even get help from someone else if I'm not around.)
Attached patch patchfile.patch (obsolete) — Splinter Review
The following patch when applied, builds without any errors in debug mode.
I have made the other edits that you mentioned.
Attachment #8908282 - Attachment is obsolete: true
Attachment #8912251 - Flags: review?(botond)
Comment on attachment 8912251 [details] [diff] [review]
patchfile.patch

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

::: xpcom/threads/RecursiveMutex.h
@@ +37,5 @@
> +  /**
> +   * AssertCurrentThreadIn
> +   **/
> +  void AssertCurrentThreadIn();
> +    //Not currently implemented. See bug 476536 for discussion.

Thanks! Just one small nit: I would put this comment into the body of AssertNotCurrentThreadIn(). Where it is right now, people could be confused whether it applies to AssertCurrentThreadIn() or AssertNotCurrentThreadIn().
Attachment #8912251 - Flags: review?(botond)
Attached patch patchfile.patchSplinter Review
Attachment #8912251 - Attachment is obsolete: true
Attachment #8912494 - Flags: review?(botond)
Comment on attachment 8912494 [details] [diff] [review]
patchfile.patch

Looks good, thanks!
Attachment #8912494 - Flags: review?(botond) → review+
I pushed the patch to the Try server to make sure it's building on all platforms and passing tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b45e4fa6329b8aa157a9cde4dde4912c4671cf5
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/412a7cc2ed9d
Use RecursiveMutex in AsyncPanZoomController.r=botond
(In reply to Botond Ballo [:botond] from comment #18)
> I pushed the patch to the Try server to make sure it's building on all
> platforms and passing tests:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0b45e4fa6329b8aa157a9cde4dde4912c4671cf5

This looked good, so I went ahead and pushed the patch to mozilla-inbound.
I also noticed you asked some questions in #apz, which I missed at the time, but I answered them after the fact:

http://logs.glob.uno/?c=mozilla%23apz#c38567

Hope that clears things up! If not, please feel free to ask more :)
https://hg.mozilla.org/mozilla-central/rev/412a7cc2ed9d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Thanks for your contribution here, Cosm!

If you're interested in working on another bug, and would like some suggestions, let me know :)
You need to log in before you can comment on or make changes to this bug.