1385335 - Use RecursiveMutex in AsyncPanZoomController

Status: RESOLVED FIXED

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

Description

[Botond Ballo [:botond]]

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.

Comment 1

[Botond Ballo [:botond]]

This might make a nice mentored bug.

Comment 2

[Cosm]

I want to work on this issue.

Comment 3

[Botond Ballo [:botond]]

(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

Comment 4

[Cosm]

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 ?

Comment 5

[Botond Ballo [:botond]]

(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

Comment 6

[Cosm]

Attachment: patchf.patch

Comment 7

[Botond Ballo [:botond]]

Cosm, did you build Firefox after writing this patch? I tried building it locally and got a number of compiler errors.

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8906548 [details] [diff] [review]
patchf.patch

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

Clearing review flag until compiler errors are fixed.

Comment 9

[Botond Ballo [:botond]]

(Assigning the bug to you to reflect that you're working on it.)

Comment 10

[Cosm]

Attachment: patchm.patch

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.

Comment 11

[Botond Ballo [:botond]]

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 12

[Botond Ballo [:botond]]

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

Comment 13

[Botond Ballo [: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.)

Comment 14

[Cosm]

Attachment: patchfile.patch

The following patch when applied, builds without any errors in debug mode.
I have made the other edits that you mentioned.

Comment 15

[Botond Ballo [: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().

Comment 16

[Cosm]

Attachment: patchfile.patch

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 8912494 [details] [diff] [review]
patchfile.patch

Looks good, thanks!

Comment 18

[Botond Ballo [:botond]]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/412a7cc2ed9d
Use RecursiveMutex in AsyncPanZoomController.r=botond

Comment 20

[Botond Ballo [: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.

Comment 21

[Botond Ballo [:botond]]

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 :)

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/412a7cc2ed9d

Comment 23

[Botond Ballo [:botond]]

Thanks for your contribution here, Cosm!

If you're interested in working on another bug, and would like some suggestions, let me know :)