Closed Bug 601547 Opened 14 years ago Closed 14 years ago

Performance regression from 3.6 when panning in GitHub network graph viewer

Categories

(Core :: Widget: Win32, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: nmanole, Assigned: tnikkel)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

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.
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
Blocks: 130078
Keywords: regression
Target Milestone: --- → Firefox 4.0
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Ever confirmed: true
Component: General → Canvas: 2D
Product: Firefox → Core
QA Contact: general → canvas.2d
Target Milestone: Firefox 4.0 → ---
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?
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?
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?
(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,
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.
OS: Windows 7 → All
Assignee: nobody → tnikkel
blocking2.0: ? → betaN+
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.
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.
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.
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.
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.
Component: Canvas: 2D → Event Handling
QA Contact: canvas.2d → events
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?
Attached patch patch (deleted) — Splinter Review
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.
Attachment #480856 - Flags: review?(jmathies)
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 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.
Attachment #480856 - Flags: review?(jmathies) → review+
Blocks: 592954, 592093
I think we should get this in ASAP; I think it could have a positive effect on other things.
Component: Event Handling → Widget: Win32
QA Contact: events → win32
I filed bug 602303 on getting a mechanism to dispatch starved paints on Linux.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Not sure why this would cause shutdown regression. What is the test even measuring?
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 :/
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#
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.
Please do!
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Depends on: 628897
Depends on: 624258
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: