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)

x86
macOS
defect
Not set
normal

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
Summary: Performance of the pdf.js regressed in Nightly → Performance of the pdf.js regressed for files with SMask graphics
Correction, FF27 is affected as well, I was actually testing on Firefox 26
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)
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?
Blocks: 925448
Component: Untriaged → Widget: Cocoa
Flags: needinfo?(smichaud)
Product: Firefox → Core
> 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)
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)
(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?
I was able to reproduce on 10.9.1 (w/ retina).
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.
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.
Assignee: nobody → smichaud
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?
Is this problem still occurring?
Flags: needinfo?(bas)
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.

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: smichaud → nobody

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.

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.

Assignee: nobody → smichaud

This is left over from when I was a Mozilla employee. I'll never get back to it.

Assignee: smichaud → nobody

(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.