Closed
Bug 972877
Opened 11 years ago
Closed 2 years ago
Performance of the pdf.js regressed for files with SMask graphics
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: yury, Unassigned)
References
()
Details
(Keywords: regression, Whiteboard: [pdfjs-c-performance])
STR:
1. Save https://bug793542.bugzilla.mozilla.org/attachment.cgi?id=663870 locally as fxpdfproblems.pdf
2. Open Firefox Nightly/30 at http://mozilla.github.io/pdf.js/web/viewer.html#pdfBug=Stats
3. Use Open File button (at the top right) to open fxpdfproblems.pdf
4. After the page finished rendering, notice "Rendering" time for Page 1
5. Close the browser
6. Open Firefox 27 at http://mozilla.github.io/pdf.js/web/viewer.html#pdfBug=Stats
7. Use Open File button (at the top right) to open fxpdfproblems.pdf
8. After the page finished rendering, notice "Rendering" time for Page 1
Actual result:
Rendering time for FF30 (step 4) is 19856ms
Rendering time for FF27 (step 8) is 2132ms
Expected result:
The rendering time shall be less or about the same for FF30 as for FF27
Reporter | ||
Updated•11 years ago
|
Summary: Performance of the pdf.js regressed in Nightly → Performance of the pdf.js regressed for files with SMask graphics
Reporter | ||
Comment 1•11 years ago
|
||
Correction, FF27 is affected as well, I was actually testing on Firefox 26
Reporter | ||
Comment 2•11 years ago
|
||
Last good nightly: 2013-11-26
First bad nightly: 2013-11-27
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=53d55d2d0a25&tochange=6ecf0c4dfcbe
(it's 28.0a1 so looks like the patch was uplifted to aurora)
Reporter | ||
Comment 3•11 years ago
|
||
The first bad revision is:
changeset: 157579:8a5c832a8bbf
user: Steven Michaud <smichaud@pobox.com>
date: Tue Nov 26 12:41:32 2013 -0600
summary: Bug 925448 - Stop CGImageRef external data being deleted prematurely. r=bgirard,bas
-----------
Steven, do you know why performance dropped about order of magnitude with your patch? Is there a way to fix that?
Reporter | ||
Updated•11 years ago
|
Keywords: regressionwindow-wanted
Comment 4•11 years ago
|
||
> Steven, do you know why performance dropped about order of magnitude with your patch?
No idea off the top of my head.
> Is there a way to fix that?
I will try to find out next week.
We should also ask Bas.
Flags: needinfo?(bas)
Comment 5•11 years ago
|
||
I'm not able to reproduce this bug.
I *did* see a regression in "Rendering" for page 1, but not nearly this bug: It was 1113ms in FF 26, but 2866ms in today's mozilla-central nightly. I tested with a plain vanilla profile on OS X 10.8.5.
I'll see if I can track down *this* performance regression.
Flags: needinfo?(smichaud)
Reporter | ||
Comment 6•11 years ago
|
||
(In reply to Steven Michaud from comment #5)
> I'm not able to reproduce this bug.
...
> I tested with a plain vanilla profile on OS X 10.8.5.
Steven, I'm using 10.9.1. Do you have Retina display on your computer?
Comment 7•11 years ago
|
||
I was able to reproduce on 10.9.1 (w/ retina).
Comment 8•11 years ago
|
||
Later I'll try on a Retina MacBook Pro, in different versions of OS X (so far I've been testing on a non-Retina Mac Pro running OS X 10.8.5).
But even a 2.5X percent regression is worth fixing. And just now I've confirmed that this 2.5X regression is somehow triggered by my patch for bug 925448 (by testing on the trunk as it was just before and after that patch landed).
Now I need to do some trial and error to find out how this could have happened.
Comment 9•11 years ago
|
||
Just discovered that the performance regression is much worse if you first maximize each page you're testing with. HiDPI mode also makes a big difference (note that you can turn this off by setting gfx.hidpi.enabled to -1).
But like I said, the bug is that there's any performance regression at all.
I'll be working on this.
Updated•11 years ago
|
Assignee: nobody → smichaud
Comment 10•11 years ago
|
||
Bas, I really need your help on this.
My first patch for bug 925448 (attachment 8335346 [details] [diff] [review]) doesn't trigger this regression. But you rejected it because, as best I can understand you, the original code (with or without my patch) allows a SourceSurfaceCGBitmapContext's corresponding SourceSurface object to be changed, though it's supposed to be immutable. We fixed this problem (in the second and final versions of my patch) by making SourceSurfaceCGBitmapContext::GetDataSurface() create a local copy of the data whenever it's called (by calling DrawTargetWillChange()). It so happened that this also fixed bug 925448.
But this is what causes the performance regression. So it looks like we have a choice between putting up with the performance regression or going back to having SourceSurface objects that aren't truly immutable.
Do you agree?
If so, which horn of the dilemma should we choose?
If not, how can we get immutable SourceSurface objects without such a large performance regression?
Comment 12•10 years ago
|
||
I'm pretty sure there's another bug dealing with the underlying problem here (the one that causes the performance problems). But I can't remember its number, and I'm unable to find it.
Comment 13•10 years ago
|
||
Found it! Bug 1001069.
Comment 14•2 years ago
|
||
The bug assignee is inactive on Bugzilla, so the assignee is being reset.
Assignee: smichaud → nobody
Comment 15•2 years ago
|
||
Sorry, there was a problem with the detection of inactive users. There was no action for a while so I won't revert the change, but feel free to reassign it to yourself in case you're still planning to work on this.
Comment 16•2 years ago
|
||
Sorry, there was a problem with the detection of inactive users. I'm reverting the change.
Assignee: nobody → smichaud
Comment 17•2 years ago
|
||
This is left over from when I was a Mozilla employee. I'll never get back to it.
Assignee: smichaud → nobody
Comment 18•2 years ago
|
||
(In reply to Steven Michaud [:smichaud] (Retired) from comment #13)
Found it! Bug 1001069.
Resolving this bug since bug 1001069 was resolved as WORKSFORME.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•