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)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: nmanole, Assigned: tnikkel)
References
()
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
patch
|
jimm
:
review+
vlad
:
approval2.0+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
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
Assignee | ||
Updated•14 years ago
|
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Ever confirmed: true
Assignee | ||
Updated•14 years ago
|
Component: General → Canvas: 2D
Product: Firefox → Core
QA Contact: general → canvas.2d
Target Milestone: Firefox 4.0 → ---
Assignee | ||
Comment 2•14 years ago
|
||
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?
Reporter | ||
Comment 3•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
(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,
Reporter | ||
Comment 6•14 years ago
|
||
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
Updated•14 years ago
|
Assignee: nobody → tnikkel
blocking2.0: ? → betaN+
Assignee | ||
Comment 7•14 years ago
|
||
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.
Reporter | ||
Comment 8•14 years ago
|
||
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.
Assignee | ||
Comment 9•14 years ago
|
||
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.
Assignee | ||
Comment 11•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Component: Canvas: 2D → Event Handling
QA Contact: canvas.2d → events
Assignee | ||
Comment 12•14 years ago
|
||
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?
Assignee | ||
Comment 13•14 years ago
|
||
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 14•14 years ago
|
||
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•14 years ago
|
||
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+
Updated•14 years ago
|
"its", not "it's" :-)
Assignee | ||
Comment 17•14 years ago
|
||
I think we should get this in ASAP; I think it could have a positive effect on other things.
Assignee | ||
Updated•14 years ago
|
Component: Event Handling → Widget: Win32
QA Contact: events → win32
Comment on attachment 480856 [details] [diff] [review]
patch
a=b7
Attachment #480856 -
Flags: approval2.0+
Assignee | ||
Comment 19•14 years ago
|
||
I filed bug 602303 on getting a mechanism to dispatch starved paints on Linux.
Assignee | ||
Comment 20•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 21•14 years ago
|
||
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 22•14 years ago
|
||
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 :/
Assignee | ||
Comment 24•14 years ago
|
||
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#
Assignee | ||
Comment 25•14 years ago
|
||
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•14 years ago
|
||
Please do!
Assignee | ||
Comment 27•14 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•