Closed
Bug 129953
Opened 22 years ago
Closed 22 years ago
Regression causing "frame drop-offs" [DHTML / painting-supression]
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.0
People
(Reporter: markushuebner, Assigned: pavlov)
References
()
Details
(Keywords: regression, testcase, top100, Whiteboard: [adt2] FIXED ON TRUNK [ETA 05/02])
Attachments
(2 files, 3 obsolete files)
14.87 KB,
patch
|
jesup
:
review+
rpotts
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
2.27 KB,
patch
|
jesup
:
review+
jesup
:
superreview+
leaf
:
approval+
|
Details | Diff | Splinter Review |
Just go to http://www.formula-one.nu/dhtml/3d.htm - in recent builds major frame drop-offs happen. Unfortunately this is also present on the 0.9.9 trunk (testing 2002031903) Tested back builds, I find out that the regression started with build 2002021803 (the testcase is fine with build 2002021708)
Reporter | ||
Updated•22 years ago
|
Reporter | ||
Comment 1•22 years ago
|
||
The checkin of bug 117061 (02/17/2002 16:10) seems to be the reason.
Blocks: 21762
Comment 2•22 years ago
|
||
Markush, please give this to the XP timer owner, pavlov. I'll help. /be
Assignee: jst → pavlov
Reporter | ||
Comment 3•22 years ago
|
||
No problems with 0.9.9 build 2002031104
Reporter | ||
Comment 4•22 years ago
|
||
A very fine testcase from Peter for playing around http://www.formula-one.nu/mozilla/timeouttest.htm You can test with different timeouts and number of elements. You can see that a small timeout causes frame-drop. You can also see that a big timeout but with many elements causes the same problem.
Comment 5•22 years ago
|
||
matic: it would help if you could produce something that measured the time between SetTimeout() and the timeout being called, and display the distribution (NOT average) of times. I suspect the problem is that for small timeouts the algorithm occasionally does some subtraction and ends up using an effective 0 timeout, causing the appearance of a jump (the reflows may get coalesced for two updates, even with the current code, before it's drawn).
Comment 6•22 years ago
|
||
jesup: I've updated the testcase with timers. (the same url) Do you think it can be used or do you want me to change it?
Comment 7•22 years ago
|
||
bug 117061 has some useful stats from an updated testcase attached. I'm duplicating a comment I just made there, since it's really more applicable to this specific bug: This test now shows we have serious problems with the SetTimeout processing, perhaps partly due to the previous attempt to fix timers. With a SetTimeout of 100ms (which is what matic used, the default settings), you shouldn't see a pattern of sometimes 250ms, sometimes 10ms. Now, the 250 may be largely due to bug 129115, but we shouldn't be seeing 10 or 20, and especially not several of them in a row. OS->All
OS: Windows 2000 → All
Hardware: PC → All
Reporter | ||
Comment 8•22 years ago
|
||
http://www.kunstforum-wien.at/index.php?area=newsletter is another really bad example. if you move the mouse-cursor over the symbol for scrolling up or down it's even hard to realise that this is animating as there seems to be no animation. if you move around with the mouse-cursor the animation takes place. as jesup already stated in bug 117061 comment #10 we are having major troubles with firing timers.
Assignee | ||
Comment 9•22 years ago
|
||
this patch makes things a lot better, but still isn't perfect..
Reporter | ||
Comment 10•22 years ago
|
||
Great - hope to see this in 1.0 as this would ease a few things.
Keywords: patch
Assignee | ||
Comment 11•22 years ago
|
||
this patch makes it up to the app's main loop code to call to turn on idle timers and makes the appshell code for mozilla on windows do it by default. other apps and platforms can make this call and do the make the calls to get this functionality, otherwise they will continue to operate as they used to.
Attachment #77190 -
Attachment is obsolete: true
Comment 12•22 years ago
|
||
I'll review this soon, by tonight for sure. I'm keen to hear from Markush and others who might try it, if they can (or get an optimized build that contains this patch from pavlov). Also, I think someone should r= -- if that's me, who will sr=? /be
Updated•22 years ago
|
Keywords: mozilla1.0 → mozilla1.0+
Assignee | ||
Comment 13•22 years ago
|
||
Updated nsAppShell.cpp patch which also processes idle timers when we are modal.
Assignee | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0
Comment 14•22 years ago
|
||
Comment on attachment 77335 [details] [diff] [review] better fix sr=rpotts@netscape.com
Attachment #77335 -
Flags: superreview+
Comment 15•22 years ago
|
||
Comment on attachment 78150 [details] [diff] [review] Updated patch to nsAppShell.cpp (previous patch required - nsAppShell changes) sr=rpotts@netscape.com
Attachment #78150 -
Flags: superreview+
Comment 16•22 years ago
|
||
Comment on attachment 77335 [details] [diff] [review] better fix r=rjesup@wgate.com
Attachment #77335 -
Flags: review+
Updated•22 years ago
|
Attachment #78150 -
Flags: review+
Comment 17•22 years ago
|
||
Comment on attachment 78150 [details] [diff] [review] Updated patch to nsAppShell.cpp (previous patch required - nsAppShell changes) r=rjesup@wgate.com
Comment 18•22 years ago
|
||
Comment on attachment 78150 [details] [diff] [review] Updated patch to nsAppShell.cpp (previous patch required - nsAppShell changes) a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #78150 -
Flags: approval+
Comment 19•22 years ago
|
||
Comment on attachment 77335 [details] [diff] [review] better fix a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #77335 -
Flags: approval+
Comment 20•22 years ago
|
||
rjesup wanted to know if I was cool with these changes, and I am -- pav already showed them to me last week -- I was just waiting for others to r=/sr= and relieve me of contributing to volunteer effect. /be
Comment 21•22 years ago
|
||
We have r/sr/a, and brendan is ok with it. Let's commit this now! If you don't have time/etc to commit it, reassign to me and I can do it.
Comment 22•22 years ago
|
||
What kind of risk is associated with this patch and how much testing has been done. Can we get some builds to QA for testing? Can you checkin on the trunk after Moz 1.0 branches and let it bake for a day or two
Comment 23•22 years ago
|
||
Time to RC1 is short. This patch is not huge and it's well reviewed. We should get this in quickly.
Assignee | ||
Comment 24•22 years ago
|
||
i've landed this on the trunk. Will land on the branch once I get adt approval.......
Comment 25•22 years ago
|
||
Let's let it bake on the trunk for a couple of days, have QA take a look at it, and make sure there are no regressions introduced, then come back to us for approval - ADT. Removing adt1.0.0. Pls renominate once it has been on the trunk a couple days, and been tested.
Keywords: adt1.0.0
Comment 26•22 years ago
|
||
I've tested the patch with 2002041003 NT4. Since I have a slow machine (PII 300), the testcase I used is a modified version of http://www.formula-one.nu/mozilla/timeouttest.htm (http://bugzilla.mozilla.org/showattachment.cgi?attach_id=78592 see bug#136688) Results show very stable numbers. Timeout 500 700 300 --------------------------------------------- Iter 1 501 701 290 " 2 501 701 301 " 3 500 701 290 " 4 501 701 291 " 5 501 701 301 " 6 500 701 300 " 7 501 701 310 " 8 501 701 311 " 9 501 701 300 " 10 500 701 310 ............................................ I'm suprised by times less than the timeout.
Assignee | ||
Comment 27•22 years ago
|
||
I'm going to be out of the office for the next week or so. I would appriciate it if someone could land this on the branch while I'm away.
![]() |
||
Comment 28•22 years ago
|
||
How much work is needed to get this fixed for non-Windows platforms at this point (per comment 11)?
Comment 29•22 years ago
|
||
Comment on attachment 77335 [details] [diff] [review] better fix these changes were backed out due to a performance regression (see bug 136677)
Attachment #77335 -
Flags: needs-work+
Comment 30•22 years ago
|
||
Comment on attachment 78150 [details] [diff] [review] Updated patch to nsAppShell.cpp (previous patch required - nsAppShell changes) marking needs-work since this was backed out (bug 136677)
Attachment #78150 -
Flags: needs-work+
Comment 31•22 years ago
|
||
Final numbers given seem to be 1% increase in Tp. For a significant improvement in DHTML functionality (not just performance) this seems worth it. DHTML functionality was broken by the previous timer checkins to reduce latency; this fixes that regression. If there are Tp issues due to that, we can resolve them or decide that they're unresolvable; we shouldn't back out a significant regression fix for 1% Tp. Note this fixes considerable randomness in timers; without this fix they can fire way earlier than requested or way later. 100ms SetTimeout was taking 10ms to 250ms, apparently randomly. I say check it back in, mirror it for Unix/etc, and then resolve any fallout.
Comment 32•22 years ago
|
||
I'm thinking the same as Randell Jesup in comment #31. Just 1%-1.5% increase in pageload for a much wanted bugfix!? I agree focus on performance is important in current Mozilla, but...
Comment 33•22 years ago
|
||
I would rather back out the whole new timer implementation that caused many of these regressions. I don't know what good came from it, but since it's probably too late to back it out we _have_ to fix the regressions so that the stuff that depends on timers are unbroken. That it cost performance sucks, but what can we do? The timers as they are right now are just not acceptable. :-(
Comment 34•22 years ago
|
||
is bug 136447 a dup of this? It got around extremely much faster after this checkin (allthough still around 5-10 times slower than MSIE)
Comment 35•22 years ago
|
||
Speed is always an issue, but in this case I would rather see Mozilla 1.0 do the right thing a little slow than see it doing the wrong thing a little more quickly.
Comment 36•22 years ago
|
||
Since these changes can be checked in in 3 independent parts, I'm going to check them in in 3 pieces to see which piece tweaked the Linux builds (since none of them should have). I just checked in the Makefile.in / makefile.win to add the nsITimerManager.idl to the build.
Comment 37•22 years ago
|
||
OK, adding the IDL, not surprisingly, didn't increase the time. So I just checked in the rest of the xpcom/threads/ changes. (Still no factory change or Windows appshell change.)
Comment 38•22 years ago
|
||
So, as I was just discussing with bbaetz & jrgm on IRC, this code *is* used by linux. The registering of the class occurs on all platforms. The change to nsXPComInit.cpp causes an extra RegisterFactory call from inside of NS_InitXPCOM2. It doesn't seem like a single class should cause a 10ms change in page load...especially since NS_InitXPCOM2 should be called way before pageload occurs but that's the only common connection that I can see.
Comment 40•22 years ago
|
||
adt1.0.0+ (on ADT's behalf) for checkin into the 1.0 branch. Pls check this in to the branch today. After it is checked in, pls add fixed1.0.0. Once QA has verified it on the branch, then add verified1.0.0.
Keywords: adt1.0.0+
Comment 41•22 years ago
|
||
Comment on attachment 78150 [details] [diff] [review] Updated patch to nsAppShell.cpp (previous patch required - nsAppShell changes) removing needs work and re-approving for the 1.0 branch
Attachment #78150 -
Flags: needs-work+
Comment 42•22 years ago
|
||
Comment on attachment 77335 [details] [diff] [review] better fix removing needs work and re-approving for the 1.0 branch.
Attachment #77335 -
Flags: needs-work+
Comment 44•22 years ago
|
||
@#$%!! The patch does not apply cleanly. It does not include mac or unix makefiles. Ugh. Attaching what I think the patch is suppose to look like. I am deeply conserned about the nsAppShell.cpp changes as they are not the same on the trunk.
Comment 45•22 years ago
|
||
Comment 46•22 years ago
|
||
after talking with dbaron, we agreed that the nsAppShell.cpp from the trunk should be landed.
Comment 47•22 years ago
|
||
Checking in widget/src/windows/nsAppShell.cpp; /cvsroot/mozilla/widget/src/windows/nsAppShell.cpp,v <-- nsAppShell.cpp new revision: 3.42.16.2; previous revision: 3.42.16.1 done Checking in xpcom/build/nsXPComInit.cpp; /cvsroot/mozilla/xpcom/build/nsXPComInit.cpp,v <-- nsXPComInit.cpp new revision: 1.126.12.2; previous revision: 1.126.12.1 done Checking in xpcom/threads/TimerThread.cpp; /cvsroot/mozilla/xpcom/threads/TimerThread.cpp,v <-- TimerThread.cpp new revision: 1.6.4.2; previous revision: 1.6.4.1 done Checking in xpcom/threads/makefile.win; /cvsroot/mozilla/xpcom/threads/makefile.win,v <-- makefile.win new revision: 1.22.2.2; previous revision: 1.22.2.1 done Checking in xpcom/threads/Makefile.in; /cvsroot/mozilla/xpcom/threads/Makefile.in,v <-- Makefile.in new revision: 1.25.4.2; previous revision: 1.25.4.1 done Checking in xpcom/threads/MANIFEST_IDL; /cvsroot/mozilla/xpcom/threads/MANIFEST_IDL,v <-- MANIFEST_IDL new revision: 3.4.4.2; previous revision: 3.4.4.1 done Checking in xpcom/threads/nsITimerManager.idl; /cvsroot/mozilla/xpcom/threads/nsITimerManager.idl,v <-- nsITimerManager.idl new revision: 1.1.4.1; previous revision: 1.1 done Checking in xpcom/threads/nsTimerImpl.cpp; /cvsroot/mozilla/xpcom/threads/nsTimerImpl.cpp,v <-- nsTimerImpl.cpp new revision: 1.11.4.2; previous revision: 1.11.4.1 done Checking in xpcom/threads/nsTimerImpl.h; /cvsroot/mozilla/xpcom/threads/nsTimerImpl.h,v <-- nsTimerImpl.h new revision: 1.5.4.2; previous revision: 1.5.4.1 done Landed the above files on the branch.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
![]() |
||
Comment 48•22 years ago
|
||
Um... are we opening other bugs for non-Windows platforms then?
Comment 49•22 years ago
|
||
Sorry but this is definitely fixed only on windows. I've opened bug 137706 for the Linux platform.
Comment 50•22 years ago
|
||
adding fised1.0.0 keyord to make sure this gets QA verification.
Keywords: fixed1.0.0
Comment 51•22 years ago
|
||
Reopening because there are problems with the patch! The nsAppShell.cpp patch caused dialog problems. Those that are familar with this patch please review the change which I landed last night. rpotts and rjesup, You reviewed these changes. Any idea what could cause 137796? Note that dbaron and I landed the trunk version of the nsAppShell.cpp.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 52•22 years ago
|
||
fixes the dialog problem
Attachment #78150 -
Attachment is obsolete: true
Attachment #79376 -
Attachment is obsolete: true
Comment 53•22 years ago
|
||
Comment on attachment 79482 [details] [diff] [review] better patch v.2 sr=alecf but I'd sure like a reviewer who's more familiar with this to read this.
Comment 54•22 years ago
|
||
Comment on attachment 79482 [details] [diff] [review] better patch v.2 r=rjesup@wgate.com - this is the same as the patch I r='d before (2nd part of pav's patch)
Attachment #79482 -
Flags: review+
Comment 55•22 years ago
|
||
Comment on attachment 79482 [details] [diff] [review] better patch v.2 AlecF sr=='d it already
Attachment #79482 -
Flags: superreview+
Comment 56•22 years ago
|
||
Comment on attachment 79482 [details] [diff] [review] better patch v.2 i'll approve this; straight backout of this file didn't build for me.
Attachment #79482 -
Flags: approval+
Comment 57•22 years ago
|
||
Checking in nsAppShell.cpp; /cvsroot/mozilla/widget/src/windows/nsAppShell.cpp,v <-- nsAppShell.cpp new revision: 3.42.16.3; previous revision: 3.42.16.2 done this should do it.
Comment 58•22 years ago
|
||
Comment on attachment 79482 [details] [diff] [review] better patch v.2 r=rpotts@netscape.com
Comment 59•22 years ago
|
||
there are residual problems with other dialogs, even with your change (cookie dialogs, for instance, require that i click on ok or cancel in rapid succession to dismiss the dialog). I don't think we can ship rc1 unless we can clean up that behaviour.
Comment 60•22 years ago
|
||
This isn't fixed. We probably need to just back it out.
Severity: major → blocker
Whiteboard: [adt2] → [adt2] BRANCH ONLY
Comment 61•22 years ago
|
||
Checking in mozilla/xpcom/threads/nsTimerImpl.h; /cvsroot/mozilla/xpcom/threads/nsTimerImpl.h,v <-- nsTimerImpl.h new revision: 1.5.4.3; previous revision: 1.5.4.2 done Checking in mozilla/xpcom/threads/nsTimerImpl.cpp; /cvsroot/mozilla/xpcom/threads/nsTimerImpl.cpp,v <-- nsTimerImpl.cpp new revision: 1.11.4.4; previous revision: 1.11.4.3 done Checking in mozilla/xpcom/threads/MANIFEST_IDL; /cvsroot/mozilla/xpcom/threads/MANIFEST_IDL,v <-- MANIFEST_IDL new revision: 3.4.4.3; previous revision: 3.4.4.2 done Checking in mozilla/xpcom/threads/Makefile.in; /cvsroot/mozilla/xpcom/threads/Makefile.in,v <-- Makefile.in new revision: 1.25.4.3; previous revision: 1.25.4.2 done Checking in mozilla/xpcom/threads/makefile.win; /cvsroot/mozilla/xpcom/threads/makefile.win,v <-- makefile.win new revision: 1.22.2.3; previous revision: 1.22.2.2 done Checking in mozilla/xpcom/threads/TimerThread.cpp; /cvsroot/mozilla/xpcom/threads/TimerThread.cpp,v <-- TimerThread.cpp new revision: 1.6.4.3; previous revision: 1.6.4.2 done Checking in mozilla/xpcom/build/nsXPComInit.cpp; /cvsroot/mozilla/xpcom/build/nsXPComInit.cpp,v <-- nsXPComInit.cpp new revision: 1.126.12.4; previous revision: 1.126.12.3 done Checking in mozilla/widget/src/windows/nsAppShell.cpp; /cvsroot/mozilla/widget/src/windows/nsAppShell.cpp,v <-- nsAppShell.cpp new revision: 3.42.16.4; previous revision: 3.42.16.3 done bash-2.05a$ backed out.
Assignee: dougt → pavlov
Status: REOPENED → NEW
Comment 62•22 years ago
|
||
Sorry for the spam, but did this get backed out properly? bonsai shows the backout applying against a branch called MOZILLA_1_0_BRANCH (not MOZILLA_1_0_0_BRANCH). Does that matter?
Comment 63•22 years ago
|
||
Removing adt1.0.0+, as this was backed out. Pls nominate again, when there is a patch that fixes the problem.
Keywords: adt1.0.0+
Comment 64•22 years ago
|
||
MOZILLA_1_0_BRANCH and MOZILLA_1_0_0_BRANCH should be aliases of each other (for now, at least).
Comment 65•22 years ago
|
||
This is no longer a branch smoketest blocker since it was backed out of the branch.
Comment 66•22 years ago
|
||
Any chance this will be fixed for 1.0, or is it just to late now that RC1 is released ? There's so much DHTML not working properly, including some stuff I made :-( Well, sorry for the spam, but I reelly really hope mozilla.org has the nesecary patience to wait for a fix for this bug. And thanks to those people who have already been working hard trying to fixing this bug.
Comment 67•22 years ago
|
||
Since this isn't fixed shouldn't it be fixed on all platforms? I don't think that there should be so major differences between behaviour on Windows, Linux and Mac versions.
Blocks: advocacybugs
Comment 68•22 years ago
|
||
There might be something related in bug 138791. I found some problems in the timer code while making it handle interval rollover correctly.
Depends on: 138791
Assignee | ||
Updated•22 years ago
|
Whiteboard: [adt2] → [adt2] FIXED ON TRUNK
Assignee | ||
Comment 69•22 years ago
|
||
The reason this wasn't changed on linux or mac is because their old timer implementations did not behave this way.
Status: NEW → ASSIGNED
Comment 70•22 years ago
|
||
Do we know exactly what is holding up this patch on the branch? This does ruin alot of DHTML websites, and it would be nice for the patch to make it into 1.0. The trunk seems to be working fine with this patch on Win. What are we looking for to get approval to land on the branch?
Comment 71•22 years ago
|
||
Timer code has been fixed, but we still have problems. Please see my comment 70 from bug 117061 for some analysis.
Assignee | ||
Comment 72•22 years ago
|
||
i think most of the remaining problems are being caused by other bugs such as the various dhtml sucking due to layout problems.
If you want to get this on the branch you should add adt1.0.0 keyword.
Comment 74•22 years ago
|
||
adding adt1.0.0 keyword... We shouldn't need Netscape's permission to check into the 1.0 branch, but it can't hurt to put this on people's radars. Like Stuart said, the problems that still remain with DHTML performance are other bugs. Fixing this one will at least get DHTML animations to work as opposed to simply stalling the browser with frame-dropoffs until the animation is finished. we have a few drivers CCd on this bug already, and it is a dependency on bug 138000 (Make RC2 not suck) so unless there are some negative issues with this patch, this and probably other related bugs (bug 117061 for example) should be checked in to the 1.0 branch.
Keywords: adt1.0.0
Comment 75•22 years ago
|
||
Doug - Considering taking this one after beta. What Top 100 sites are exhibiting a problem with this bug?
Reporter | ||
Comment 76•22 years ago
|
||
This bug is a major regressions causing most DHTML sites to hang and many DHTML examples have frame-drop-offs. It has already done fine on the trunk - so why not checking into 1.0 branch?!
Comment 77•22 years ago
|
||
This is a stopship for RC2 (that's why it's blocking 138000). It's been on the trunk for a long time with nothing but good results. We must get this landed before tomorrow.
Comment 78•22 years ago
|
||
rolling the dice ... adt1.0.0+ (on ADT's behalf) approval for checkin to the 1.0 branch. Pls check this in to 1.0 branch today, then add the fixed1.0.0 keyword.
Assignee | ||
Comment 79•22 years ago
|
||
marking fixed. now on trunk and branch.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago → 22 years ago
Keywords: fixed1.0.0
Resolution: --- → FIXED
Comment 80•22 years ago
|
||
I must say, that lastest trunk for Windows is amazing. Amazing results with moving layers in most combinations (with clipping etc.). The problem still apears when Mozilla has to show many, many layers (bug #140789 ). I hope this patch wont be exluded in RC2?
Reporter | ||
Comment 81•22 years ago
|
||
It's on the 1.0 branch, so ready for RC2 :) Fixing bug 102321 helped many, many things - also a bit doing animations over a background image. Nevertheless bug 139986, bug 87808, bug 107746 still need our attention.
![]() |
||
Comment 82•22 years ago
|
||
*** Bug 142399 has been marked as a duplicate of this bug. ***
Comment 83•18 years ago
|
||
The patch in this bug causes a pretty serious problem (see bug 291386). By suppressing timers until the windows event loop is idle, the timer thread's filter gets greatly skewed, and as a result, the timer thread ends up over-correcting like mad for the skew. It results in timers firing at 1ms intervals post page load, for example. Unfortunately, all of the testcases linked from this bug are now 404s. Can anyone help resurrect a testcase for this bug? Thanks!
Blocks: 291386
Reporter | ||
Comment 84•18 years ago
|
||
The former testcases of formula-one.nu are at http://wd-testnet.world-direct.at/mozilla/dhtml/funo/
Comment 85•18 years ago
|
||
Markus: Thanks!
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•