Closed Bug 1158557 Opened 9 years ago Closed 9 years ago

Significant performance regression in Print Preview

Categories

(Core :: Print Preview, defect)

40 Branch
Unspecified
Windows 7
defect
Not set
normal

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)

Attached file fw4.pdf
[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)
Presumably pdf.js is using RequestAnimationFrame. Though I'm still not sure why we're not considering the relevant iframe to be painted.
Attached file Communique_Commun.pdf
another sample
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)
(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)
Keywords: qaurgent, qawanted
(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)
(You said "This happens with/without e10s." in comment 0, but that is not my experience.)
Do we make iframe visibility determinations differently on different platforms?  What platforms were you each testing on?
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.
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.
(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.)
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: nobody → seth
Status: NEW → ASSIGNED
Depends on: 1160058
(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.
(In reply to Alice0775 White from comment #13)
> Okay, I've filed a new Bug 1160058.

Thanks!
No longer depends on: 1160058
See Also: → 1160058
Updated the patch to take account of the fact that
nsIDocument::mOriginalDocument gets dropped during cycle collection.
Attachment #8600095 - Flags: review?(mstange)
Attachment #8599667 - Attachment is obsolete: true
Attachment #8599667 - Flags: review?(mstange)
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 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+
Thanks for the review! I'll make those changes.
Here's an updated version of the patch that includes the changes requested in
comment 17.
Attachment #8600095 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/352eda4d6a05
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Don't need to track it since it's fixed (removed flag for 40).
Florin can your team have a look and verify this fix? Thanks!
Flags: needinfo?(florin.mezei)
(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
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.
Status: RESOLVED → VERIFIED
Keywords: qaurgent, qawanted
You need to log in before you can comment on or make changes to this bug.