601547 - Performance regression from 3.6 when panning in GitHub network graph viewer

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Adrian Sampaleanu]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/7.0.541.0 Safari/534.10
Build Identifier: 4.0b7pre built from http://hg.mozilla.org/mozilla-central/rev/bb18e81ea0f8

Performance while panning in the GitHub network graph viewer (see http://github.com/sinatra/sinatra/network for example) has seriously degraded from that seen in Firefox 3.6.x. and the graph viewer is hardly usable.

The nightly Chromium also displays great performance. The GitHub site has very wide visibility and this issue should not be left unresolved. 

Reproducible: Always

Steps to Reproduce:
1. Go to the network viewer page of any project on GitHub (for example, http://github.com/sinatra/sinatra/network) and attempt to pan the graph by clicking and dragging. 

Actual Results:  
The graph viewer updates so slowly that usability is severely impaired.

Expected Results:  
The graph should pan smoothly. See Firefox 3.6 or Chromium for an example of this.

Comment 1

[Alice0775 White]

Regession window:
Works:
http://hg.mozilla.org/mozilla-central/rev/01fa971e62ee
Mozilla/5.0 (Windows NT 6.1; en-US; rv:1.9.2.10) Gecko/20100915 Firefox/4.0b5pre ID:20100827190212
Fails:
http://hg.mozilla.org/mozilla-central/rev/0886ad6e6aaa
Mozilla/5.0 (Windows NT 6.1; en-US; rv:1.9.2.10) Gecko/20100915 Firefox/4.0b5pre ID:20100827190555
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=01fa971e62ee&tochange=0886ad6e6aaa

The following changeset causes the problem
7804b5cf4313	Robert O'Callahan — Bug 130078. Part 2. Remove widgets from all subframes. r=dbaron

Comment 2

[Timothy Nikkel (:tnikkel)]

Profiling this shows about 30% of the time is in canvas stuff, 12% is in cairo stuff. Not sure how bug 130078 affected this. The transparency or not of the window doesn't make a difference. Vlad, any insight into what is going on here?

Comment 3

[Adrian Sampaleanu]

The canvas content for the example URL is:

<canvas height="600" width="920" style="z-index: 0; cursor: pointer;">
      <object width="920" height="600" id="network" classid="clsid:d27cdb6e-ae6d-11cf-96b8-444553540000">
      <param value="http://assets1.github.com/flash/network-3.0.0.swf?2e6220cadf38492df21df2789ab5ee6bd23f433d" name="movie">
      <param value="always" name="allowScriptAccess">
      <param value="high" name="quality">
      <param value="noscale" name="scale">
      <param value="repo=sinatra/sinatra" name="FlashVars">
      <embed width="920" height="600" flashvars="repo=sinatra/sinatra" pluginspage="http://www.macromedia.com/go/getflashplayer" type="application/x-shockwave-flash" allowscriptaccess="always" quality="high" name="network" src="http://assets1.github.com/flash/network-3.0.0.swf?2e6220cadf38492df21df2789ab5ee6bd23f433d">
      </object>
</canvas>

Is it possible there is a rendering conflict between the Firefox rendering code and the embedded Flash player considering that the specified Firefox version and the latest Flash player both try to control the graphics hardware for acceleration?

Comment 4

[Alice0775 White]

Even if I disabled flash plugin,the scroll of the canvas is not improved.

BTW,
It is ok to scroll canvas by keyboard shortcut  h,j,k,l.
So, there is a problem for the handling of mouse event?

Comment 5

[Alice0775 White]

(In reply to comment #4)
> BTW,
> It is ok to scroll canvas by keyboard shortcut  h,j,k,l.

j and k are not work,

Comment 6

[Adrian Sampaleanu]

I just noticed that if you click and drag to pan and you move only a few (maybe 5?) pixels from the mousedown location, the graph seems to refresh reasonably fast. So, it looks like the event queue is being flooded with greater movements and the viewer can't keep up with all the events.

Comment 7

[Timothy Nikkel (:tnikkel)]

So I'm going to guess that mouse move events are getting processed before paint events, and so paint events are getting starved here because in bug 592954 I saw that mouse events seemed to be processed before paint events there too. Not sure why that would change though.

Comment 8

[Adrian Sampaleanu]

It looks like keyboard events due to auto-repeating have the same effect. If you hold down the arrow keys, paint updates happen pretty much only when you release the keys. If you instead use short, staccato key-presses, updates can keep up with the individual key-presses even if you do them quite fast.

Comment 9

[Timothy Nikkel (:tnikkel)]

This bug seems to exist on Linux going back at least two years. But it doesn't seem to happen in the Ubuntu packaged versions of Firefox.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yeah, looks like a weird event issue -- if I pan slowly it updates quickly, as mentioned in comment #8.  Not sure if it's a specific canvas thing, looking at the actual events that are processed would probably be helpful.

Comment 11

[Timothy Nikkel (:tnikkel)]

I looked through the diff Ubuntu provides of what they ship from what we provide on this page, and I didn't see anything that could be related to event processing.

Comment 12

[Timothy Nikkel (:tnikkel)]

This bug and bug 592954 seem to be caused by mouse move events getting processed first and painting being starved because it gets processed after them. Is there something about events being sent to a child vs toplevel widget that would change something like that?

Comment 13

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Our code on Windows for dispatching starved paints became (more or less) a no-op when bug 130078 landed because we used EnumChildWindows to call DispatchStarvedPaints on all child windows. But this skips the parent window itself (which is basically the only window we care about now).

This fixes this bug as well as bug 592954 and bug 592093, on Windows at least. The Linux problem is much older and probably needs a similar fix to what we've had on Windows for a while. I don't know what the situation is on Mac.

Comment 14

[[e-mail]]

Comment on attachment 480856 [details] [diff] [review]
patch

>-    // it's children.
>+    // it's children (but not the window itself).
Any chance of a grammar fix before its checkin? ;-)

Comment 15

[Jim Mathies [:jimm]]

Comment on attachment 480856 [details] [diff] [review]
patch

r+ with the comment touch up neil mentioned. ('the' maybe?)

bug 513162 also probably plays into this since we removed the main child widget and started painting in the top level window.

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

"its", not "it's" :-)

Comment 17

[Timothy Nikkel (:tnikkel)]

I think we should get this in ASAP; I think it could have a positive effect on other things.

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 480856 [details] [diff] [review]
patch

a=b7

Comment 19

[Timothy Nikkel (:tnikkel)]

I filed bug 602303 on getting a mechanism to dispatch starved paints on Linux.

Comment 20

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/d7b6ab26d39e

Comment 21

[Timothy Nikkel (:tnikkel)]

Had to back this out due to Tshutdown regressions on Windows (on tip and relbranch).

http://hg.mozilla.org/mozilla-central/rev/bb15ce71d4d1
http://hg.mozilla.org/mozilla-central/rev/1dc529910f89
http://hg.mozilla.org/mozilla-central/rev/aa89112dfbfd
http://hg.mozilla.org/mozilla-central/rev/076923aa120a

Comment 22

[Timothy Nikkel (:tnikkel)]

Not sure why this would cause shutdown regression. What is the test even measuring?

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

It's possible that this is causing us to do more repaints during shutdown, as we tear things down.  I wouldn't have backed it out just due to a Tshutdown regression :/

Comment 24

[Timothy Nikkel (:tnikkel)]

I looked at the old tree-management archives and there was indeed an improvement in Tshutdown when bug 513162 landed. http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/5719897e1ed074e4#

Comment 25

[Timothy Nikkel (:tnikkel)]

Ok, so I ran this test on Windows 7 and I get no paints events whatsoever reaching the view manager. The test opens a window and closes it immediately (visually it is very quick, all I see is a white window), so that is not too surprising. If I try using the computer while the test is running I get some paint events, probably because my input is causing the paints to happen. So something is slightly different on the talos boxes that causes a paint to happen.

Anyways, since we get no paints events at all, that means the startup portion of the test (tested in the same test) isn't including the first paint of the browser, which I don't think is ideal for a startup test. As expected there is an invalidate for the whole window posted during the startup portion, but it doesn't get served. This patch probably makes us hit that first paint during the shutdown portion for some reason. I don't think this represents a real regression users will see as I think browser sessions where no painting happens until the users closes the browser are rare and should not be optimized for.

If there are no objections I am going to reland this and take the hit in the numbers.

Comment 26

[Joe Drew (not getting mail)]

Please do!

Comment 27

[Timothy Nikkel (:tnikkel)]

Pushed again
http://hg.mozilla.org/mozilla-central/rev/966b58fd08ed