Closed
Bug 589396
Opened 14 years ago
Closed 12 years ago
Tgfx is broken and 130078 exposes it
Categories
(Testing :: Talos, defect)
Testing
Talos
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: tnikkel, Unassigned)
References
Details
Attachments
(1 file)
(deleted),
patch
|
anodelman
:
review+
|
Details | Diff | Splinter Review |
Tgfx_nochrome absolutely blows up with bug 130078, see the links in bug 588663. There are 10000%+ increases.
The pageloader extension in pageloader.js picks a window with this code in runRenderTest
if (browserWindow)
win = content.contentWindow;
else
win = window;
and then calls nsIDOMWindowUtils::Redraw 500 times on win. For Tgfx_chrome the if condition is always true, for Tgfx_nochrome it is always false due to how the test is setup. In the Tgfx_chrome case win contains the pages we are measuring the performance of (ie borders-dashed.html). In the Tgfx_nochrome case win is the parent chrome window (ie pageloader.xul) which contains a child document that loads the pages we are testing.
Now, all nsIDOMWindowUtils::Redraw does is get the root frame of the window and call InvalidateWithFlags(r, nsIFrame::INVALIDATE_IMMEDIATE) on it where r is the size of the root frame. nsIFrame::InvalidateWithFlags checks if painting is suppressed in the presshell, and if it is returns and doesn't do anything.
The 500 nsIDOMWindowUtils::Redraw calls happen off of the load event for the child content document. DocumentViewerImpl::LoadComplete is where the load event is dispatched, and it unsuppresses painting after it dispatches the load event.
So in Tgfx_chrome case the 500 nsIDOMWindowUtils::Redraw calls happen off of the load event of the child content document, and are targeted at the child content document. So when they happen painting is suppressed and they all do nothing.
In the Tgfx_nochrome case the 500 nsIDOMWindowUtils::Redraw calls happen off of the load event of the child content document, but are targeted at the root chrome document. The root chrome document has not just loaded so its painting is not suppressed. But there is nothing to paint in the root chrome document, so it is still quick. This explains why Tgfx_nochrome takes longer than Tgfx_chrome.
When we add the patches from bug 130078 the view managers for the chrome and content documents are linked, so the paint events targeted at the root chrome document also paint the child content document (we don't currently suppress painting when entering a subdocument that has painting suppressed). And this takes a lot longer since it is actually doing some work.
Clearly we need to fix Tgfx, but I don't think this should block 130078 landing.
Reporter | ||
Comment 2•14 years ago
|
||
For a quick "fix" we can just restore the broken behaviour so the numbers stay about the same by making the window we send the Redraw's to be the content window in the nochrome case as well.
Comment 3•14 years ago
|
||
Is this a version of bug 493050 ?
Reporter | ||
Comment 4•14 years ago
|
||
Closely related. If we don't change anything then bug 130078 is going to massively regress the Tgfx_nochrome numbers. Bug 130078 needs to land soon. So we need to do something about that. We can either disable the test. Or we can make a small change to the testing harness so that it continues to be bogus, but gives approximately the same numbers. We can decide what we want to do with Tgfx longer term in bug 493050, but we need a quick solution here.
Comment 5•14 years ago
|
||
I think spending any time at all making Tgfx results look sane is time wasted. Send an email to dev.tree-management saying "please ignore this apparent Tgfx regression" and move on. We can work out what to do with Tgfx in bug 493050.
Comment 6•14 years ago
|
||
I would also suggest that we should disable tgfx till it actually does something useful - right now we are just wasting cpu time on running it for every branch on every OS for every push.
Yes please, can you do that?
Comment 8•14 years ago
|
||
Attachment #471348 -
Flags: review?(anodelman)
Updated•14 years ago
|
Attachment #471348 -
Flags: review?(anodelman) → review+
Comment 9•14 years ago
|
||
Comment on attachment 471348 [details] [diff] [review]
Disable tgfx
Checked in as changeset: 2932:c0b16bcdc30b, and masters reconfigured.
Reporter | ||
Comment 10•14 years ago
|
||
Should we track fixing Tgfx in bug 493050 and mark this resolved?
Comment 11•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•