Closed
Bug 1158557
Opened 10 years ago
Closed 10 years ago
Significant performance regression in Print Preview
Categories
(Core :: Print Preview, defect)
Tracking
()
VERIFIED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox39 | --- | unaffected |
firefox40 | --- | verified |
People
(Reporter: alice0775, Assigned: seth)
References
Details
(Keywords: perf, regression)
Attachments
(3 files, 2 obsolete files)
(deleted),
application/pdf
|
Details | |
(deleted),
application/pdf
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
[Tracking Requested - why for this release]: performance regression
This happens with/without e10s.
Steps to reproduce:
1. Open attached
2. Open Print Preview(Alt > File > Print Preview)
Actual Results:
Rendering is super sloooooooooooow. it takes more than 60sec/2page
Expected Results:
Rendering should be within 1-5sec/2page.
Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e2009551c1be&tochange=2dfcc10c7270
Regressed by: 19a79b7400fe Seth Fowler — Bug 1145439 (Part 1) - Throttle requestAnimationFrame for non-visible iframes. r=mstange,mchang
Flags: needinfo?(seth)
Assignee | ||
Comment 1•10 years ago
|
||
Presumably pdf.js is using RequestAnimationFrame. Though I'm still not sure why we're not considering the relevant iframe to be painted.
Reporter | ||
Comment 2•10 years ago
|
||
another sample
Assignee | ||
Comment 3•10 years ago
|
||
OK, I've investigated this and I've concluded:
- We are not throttling rAF for any documents in this testcase.
- Reverting bug 1145439 does not fix the bug.
Therefore I must conclude that the regression range is wrong.
Flags: needinfo?(seth)
Reporter | ||
Comment 4•10 years ago
|
||
(In reply to Seth Fowler [:seth] from comment #3)
> OK, I've investigated this and I've concluded:
>
> - We are not throttling rAF for any documents in this testcase.
>
> - Reverting bug 1145439 does not fix the bug.
>
> Therefore I must conclude that the regression range is wrong.
???
Why do not you check with m-i tinder-box build.
The regression range is correct. I have 100% confidence.
Maybe you do not follow STR.
You should not use "New e10s Window" nor "New non-e10s Window" from File menu.
Flags: needinfo?(seth)
Reporter | ||
Updated•10 years ago
|
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Alice0775 White from comment #4)
> ???
> Why do not you check with m-i tinder-box build.
> The regression range is correct. I have 100% confidence.
>
> Maybe you do not follow STR.
I'm pretty sure I'm following the STR perfectly.
Here's what mozregression gave me:
Last good revision: 2114ef80f6ae (2014-11-06)
First bad revision: b62ccf3228ba (2014-11-07)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2114ef80f6ae&tochange=b62ccf3228ba
Pretty sure the culprit is:
a75897e664dd Felipe Gomes — Bug 1093691 - Enable e10s by default on Nightly (not riding trains), and activate infobar notice. r=gavin a=RyanVM on a CLOSED TREE
I tested the STR with e10s disabled and enabled, and disabling e10s fixes the issue.
Flags: needinfo?(seth)
Assignee | ||
Comment 6•10 years ago
|
||
(You said "This happens with/without e10s." in comment 0, but that is not my experience.)
Comment 7•10 years ago
|
||
Do we make iframe visibility determinations differently on different platforms? What platforms were you each testing on?
Assignee | ||
Comment 8•10 years ago
|
||
I just investigated that possibility, and it appears that Windows behaves differently than the other platforms for some reason. Disabling e10s still results in poor performance on current Nightly on Windows, whereas on other platforms that eliminates the problem. Let me investigate a little more - there must be something platform-specific here.
Assignee | ||
Comment 9•10 years ago
|
||
Alright, I've confirmed that the behavior *is* different on different platforms. On Windows we perceive the "real" document as not painted - we're only painting the static clone document - but rAF is running against the real document and so it gets throttled.
This is a straightforward fix. Patch coming.
Assignee | ||
Comment 10•10 years ago
|
||
(FWIW, the e10s issue seems to be an entirely separate bug. It confused me into thinking I had reproduced the issue reported in this bug, which threw off my regression checking entirely.)
Assignee | ||
Comment 11•10 years ago
|
||
Here's the patch. We just count the number of live static clones each document
has, and don't throttle documents which have any.
Attachment #8599667 -
Flags: review?(mstange)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → seth
Status: NEW → ASSIGNED
Assignee | ||
Comment 12•10 years ago
|
||
Reporter | ||
Comment 13•10 years ago
|
||
(In reply to Seth Fowler [:seth] from comment #5)
> (In reply to Alice0775 White from comment #4)
> > ???
> > Why do not you check with m-i tinder-box build.
> > The regression range is correct. I have 100% confidence.
> >
> > Maybe you do not follow STR.
>
> I'm pretty sure I'm following the STR perfectly.
>
> Here's what mozregression gave me:
>
> Last good revision: 2114ef80f6ae (2014-11-06)
> First bad revision: b62ccf3228ba (2014-11-07)
> Pushlog:
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=2114ef80f6ae&tochange=b62ccf3228ba
>
> Pretty sure the culprit is:
>
> a75897e664dd Felipe Gomes — Bug 1093691 - Enable e10s by default on Nightly
> (not riding trains), and activate infobar notice. r=gavin a=RyanVM on a
> CLOSED TREE
>
> I tested the STR with e10s disabled and enabled, and disabling e10s fixes
> the issue.
Okay, I've filed a new Bug 1160058.
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Alice0775 White from comment #13)
> Okay, I've filed a new Bug 1160058.
Thanks!
Assignee | ||
Comment 15•10 years ago
|
||
Updated the patch to take account of the fact that
nsIDocument::mOriginalDocument gets dropped during cycle collection.
Attachment #8600095 -
Flags: review?(mstange)
Assignee | ||
Updated•10 years ago
|
Attachment #8599667 -
Attachment is obsolete: true
Attachment #8599667 -
Flags: review?(mstange)
Comment 16•10 years ago
|
||
Comment on attachment 8600095 [details] [diff] [review]
Don't throttle rAF for documents with live static clones
I'm not comfortable reviewing this patch. Redirecting to smaug.
Attachment #8600095 -
Flags: review?(mstange) → review?(bugs)
Comment 17•10 years ago
|
||
Comment on attachment 8600095 [details] [diff] [review]
Don't throttle rAF for documents with live static clones
Could you add a helper method to nsDocument which checks
if we're in static document, and if so, and if we have mOriginalDocument,
decreases mStaticCloneCount.
Something like
void UnlinkStaticDocumentFromOriginalDocument()
{
if (IsStaticDocument() && mOriginalDocument) {
MOZ_ASSERT(mOriginalDocument->mStaticCloneCount > 0);
mOriginalDocument->mStaticCloneCount--;
mOriginalDocument = nullptr;
}
}
And call that both in unlink and in document's dtor.
(It is possible that cycle collectable object's unlink isn't ever called if refcnt drops to zero without CC doing any work!)
We could also do this all in presentation level (increase the counter when presshell for static document is created and decrease when it is destroyed), that way this would depend less on
cycle collector. But either way.
Attachment #8600095 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 18•10 years ago
|
||
Thanks for the review! I'll make those changes.
Assignee | ||
Comment 19•10 years ago
|
||
Here's an updated version of the patch that includes the changes requested in
comment 17.
Assignee | ||
Updated•10 years ago
|
Attachment #8600095 -
Attachment is obsolete: true
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Comment 21•10 years ago
|
||
Comment 22•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 23•10 years ago
|
||
Don't need to track it since it's fixed (removed flag for 40).
tracking-firefox40:
? → ---
Florin can your team have a look and verify this fix? Thanks!
Flags: needinfo?(florin.mezei)
Comment 25•10 years ago
|
||
(In reply to Liz Henry (:lizzard) from comment #24)
> Florin can your team have a look and verify this fix? Thanks!
Assigning Cornel here for verification.
Flags: needinfo?(florin.mezei) → qe-verify+
QA Contact: cornel.ionce
Comment 26•10 years ago
|
||
Reproduced this issue on Windows 7 64-bit using 2015-04-25 Nightly, build ID: 20150425030208.
Verified the fix on:
- latest Nightly 41.0a1, build ID: 20150518030202
- latest Aurora 40.0a2, build ID: 20150518004004.
Print preview takes no more than 2 seconds to render the attached pdfs.
You need to log in
before you can comment on or make changes to this bug.
Description
•