1026009 - pdf.js takes 75s to render the last page of this PDF whereas Chrome takes 10s - clip() is slow

Status: REOPENED

(Core :: Graphics: Canvas2D, defect)

Description

[Ed Morley [:emorley]]

STR:
1) Visit http://www.richmond.gov.uk/conarea55_a3_rgb.pdf
2) Scroll immediately to the last page
3) Wait for the diagram/map to finish rendering

Expected:
Time taken for 2+3 to be comparable to other PDF viewers, and ideally <10s.

Actual:
In latest nightly it takes 75 seconds, compared to 10 seconds in Chrome dev.latest.

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Built from https://hg.mozilla.org/mozilla-central/rev/a6a457fe2a2a

Comment 1

[Ed Morley [:emorley]]

Occurs with a new profile too.

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

Confirmed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 ID:20140617080437 CSet: e7e9304438df

"PDF 9671ea32c42411dc91670112476331a [1.6 Adobe PDF library 7.77 / Adobe Illustrator CS2] (PDF.js: 1.0.389)"

Comment 3

[Ed Morley [:emorley]]

Attachment: Testcase

Attaching PDF in case it stops being available.

Comment 4

[Ed Morley [:emorley]]

Attachment: testcase-profile

Yury, would you mind taking a look at this?

The profile (attached) says we spend 57% of the time under InternalRenderTask__next() -> CanvasGraphics_executeOperatorList():
https://github.com/mozilla/pdf.js/blob/0f0f0688a14cd70d687f04005d59ee5410a10b73/src/display/api.js#L1386

Which breaks down further into two pretty equal parts:

CanvasGraphics_stroke() [24% of total profile]:
https://github.com/mozilla/pdf.js/blob/11302f09a46a046433513009bb246dbc0e579329/src/display/canvas.js#L1030

CanvasGraphics_consumePath() [23% of total profile]:
https://github.com/mozilla/pdf.js/blob/11302f09a46a046433513009bb246dbc0e579329/src/display/canvas.js#L2070

Also, it's worth noting that the profile graph is sawtooth, with sizable gaps with no activity (I guess this explains the no more than 5% CPU usage by Nightly whilst loading this PDF).

Comment 5

[Yury Delendik (:yury)]

Attachment: html test that relfects operations in pdf

Comment 6

[Yury Delendik (:yury)]

The same problem exists on Mac OSX too. I created a test (see attachment 8442120 [details]) that performs the same exact operations as in the specified pdf. There are about 250,000 operations and most of them like that.

On FF Windows 7, it takes "9725ms" and on Chrome "85ms". On FF Mac OSX, it takes "2809ms" and on Chrome "66ms". Looks like it is our graphics backend limitations, moving to the different compartment.

Comment 7

[Michael Bebenita [:mbx]]

(In reply to Yury Delendik (:yury) from comment #6)
> The same problem exists on Mac OSX too. I created a test (see attachment
> 8442120 [details]) that performs the same exact operations as in the
> specified pdf. There are about 250,000 operations and most of them like that.
> 
> On FF Windows 7, it takes "9725ms" and on Chrome "85ms". On FF Mac OSX, it
> takes "2809ms" and on Chrome "66ms". Looks like it is our graphics backend
> limitations, moving to the different compartment.

On Nightly Mac OSX "7954ms" with Core Graphics, "35ms" with Skia.

Comment 8

[Ed Morley [:emorley]]

I can repro this on latest nightly; any news on this? :-)

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

We're moving to Skia on other platforms, but the D2D is still slower than Skia in this case.  It's all in the clip() call.  Bas, would you expect the push clip stuff you're working on to help here?

Comment 10

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #9)
> We're moving to Skia on other platforms, but the D2D is still slower than
> Skia in this case.  It's all in the clip() call.  Bas, would you expect the
> push clip stuff you're working on to help here?

I suspect not but I will test. I think this is mostly pdfjs doing something fairly inefficient here but I'll need to see. If these are rectangular clips part of the problem is I think we treat all clips in Canvas as complex clips, we may be able to change that if we think that's worth it.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Bas Schouten (:bas.schouten) from comment #10)
> ...
> I suspect not but I will test. I think this is mostly pdfjs doing something
> fairly inefficient here but I'll need to see. If these are rectangular clips
> part of the problem is I think we treat all clips in Canvas as complex
> clips, we may be able to change that if we think that's worth it.

The "html test..." attachment above is an extraction of operations.  This takes ~8 seconds to run in Firefox, and sub 50ms in others.  Four sided clips, half of them rectangles, half of them four sided polys.  Both take about the same time, so as you pointed out, we're not special casing rectangular clips.

pdf.js could still be doing something suboptimal, but we do have a benchmark that doesn't quite does what it should be.

Mason, can you take a look at what it would take to special case rectangular clips?

Comment 12

[Yury Delendik (:yury)]

> pdf.js could still be doing something suboptimal

PDF.js is just converting PostScript commands to canvas 2d calls. So best PDF viewer can do is to recognize some patterns and issue faster canvas 2d calls. Question will be: what patterns are bad and what shall be used instead?

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

In this case, I'd like to say "use rectangles instead of paths for clipping, when the path happens to be a rectangle", but that won't make a difference until we actually optimize for rectangle clipping.

Comment 14

[Mason Chang [Inactive] [:mchang]]

Updated profile:

http://people.mozilla.org/~bgirard/cleopatra/#report=96d64ddf5843526f666172fd687262c7a6f37067

I can only reproduce this on Windows. On Mac, it seems fine.

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Mason Chang [:mchang] from comment #14)
> 
> I can only reproduce this on Windows. On Mac, it seems fine.

Yes, Skia (on Mac) is fine; it's slow with D2D on accelerated Windows.

Comment 16

[Mason Chang [Inactive] [:mchang]]

The test case can be made a lot faster if we optimize for clipping rectangles instead of paths I think. Looking at the test case, I changed these lines:

ctx.beginPath();
ctx.moveTo(27.847,27.843);
ctx.lineTo(27.847,813.04);
ctx.lineTo(1161.705,813.066);
ctx.lineTo(1161.703,27.854);
ctx.closePath();

To:
ctx.rect(27.847, 27.843, 1161.705 - 27.847, 813.066 - 27.854);

I'm not sure if this is actually legal, since it's technically a floating point rounding error, or if this was meant to be a rect but just had some typos. Was this the case?

Do you know if pdf.js could convert such calls to Rect() calls easily? Then we could speed it up on the backend side of things. Thanks!

Comment 17

[Mason Chang [Inactive] [:mchang]]

Locally, clipping rects on both the front end and in the calls to the DrawTarget made the test case load instantly like Chrome.

Comment 18

[Yury Delendik (:yury)]

Mason,

It's not always legal, the PDF rectangle command are a little bit different and doesn't have beginPath(), see http://www.adobe.com/content/dam/Adobe/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#page=141&zoom=140,-29,629 . So in some cases we might not know if beginPath was implied.

Comment 19

[Ed Morley [:emorley]]

(Cancelling needinfo since I'm the bug reporter, rather than someone working on Canvas or pdf.js)

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Mason Chang [:mchang] from comment #16)
> The test case can be made a lot faster if we optimize for clipping
> rectangles instead of paths I think. Looking at the test case, I changed
> these lines:
> 
> ctx.beginPath();
> ctx.moveTo(27.847,27.843);
> ctx.lineTo(27.847,813.04);
> ctx.lineTo(1161.705,813.066);
> ctx.lineTo(1161.703,27.854);
> ctx.closePath();
> 
> To:
> ctx.rect(27.847, 27.843, 1161.705 - 27.847, 813.066 - 27.854);
> 

This is changing the result, though by very little.  The above path is not a rectangle, unless we drop some precision.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

Actually, one thing I didn't understand - does replacing the rectangular path with a rectangle help the performance?  In other words, change this:

ctx.beginPath();
ctx.moveTo(27.847,27.843);
ctx.lineTo(27.847,813.04);
ctx.lineTo(1161.705,813.04);
ctx.lineTo(1161.705,27.843);
ctx.closePath();

to this:

ctx.rect(27.847, 27.843, 1161.705 - 27.847, 813.066 - 27.854);


Because, if it does, we should be able to fix that.

Comment 22

[Mason Chang [Inactive] [:mchang]]

(In reply to Milan Sreckovic [:milan] from comment #20)
> (In reply to Mason Chang [:mchang] from comment #16)
> > The test case can be made a lot faster if we optimize for clipping
> > rectangles instead of paths I think. Looking at the test case, I changed
> > these lines:
> > 
> > ctx.beginPath();
> > ctx.moveTo(27.847,27.843);
> > ctx.lineTo(27.847,813.04);
> > ctx.lineTo(1161.705,813.066);
> > ctx.lineTo(1161.703,27.854);
> > ctx.closePath();
> > 
> > To:
> > ctx.rect(27.847, 27.843, 1161.705 - 27.847, 813.066 - 27.854);
> > 
> 
> This is changing the result, though by very little.  The above path is not a
> rectangle, unless we drop some precision.

Yes, the above path is not a rectangle. I wasn't sure if this was by design or a typo, and just wanted to confirm.


(In reply to Milan Sreckovic [:milan] from comment #21)
> Actually, one thing I didn't understand - does replacing the rectangular
> path with a rectangle help the performance?  In other words, change this:
> 
> ctx.beginPath();
> ctx.moveTo(27.847,27.843);
> ctx.lineTo(27.847,813.04);
> ctx.lineTo(1161.705,813.04);
> ctx.lineTo(1161.705,27.843);
> ctx.closePath();
> 
> to this:
> 
> ctx.rect(27.847, 27.843, 1161.705 - 27.847, 813.066 - 27.854);
> 
> 
> Because, if it does, we should be able to fix that.

Locally, if we force it to be a rect (incorrectly, but just for testing purposes), and support PushClipRect() in Canvas to directly call DrawTarget::PushClipRect, it does become a lot faster. But the test looks like it was meant to not be a rect, so it wouldn't work on the actual test pdf. IE is fast, so we're going to dig and see what IE is doing.

Comment 23

[Mason Chang [Inactive] [:mchang]]

Attachment: Visual Studio concurrency profiles

Edge has multiple content processes, so I profiled I think the parent process + content process (The PIDs keep changing everytime I refresh VS). This profile contains 2 edge profiles and 1 nightly profile (e10s disabled).

Comment 24

[Mason Chang [Inactive] [:mchang]]

One note about the profiles: The nightly profile was just loading the page once. The Edge profiles are me constantly hitting refresh since the page loads quickly.

Comment 25

[Jeff Muizelaar [:jrmuizel]]

Attachment: A cleaned up version of the Edge Visual Studio concurrency profiles

This shows that Edge is using CombineWithGeometry for clipping with canvas. We could experiment with the approach with some difficulty.

Comment 26

[Mason Chang [Inactive] [:mchang]]

Attachment: Optimize push clip rects if the path is a rect

If a path is a rect, we'll use drawTarget->PushClipRect instead. Both CG / Skia have built in support for detecting if a path is a rect. D2D forces you to replay the geometry to detect if a rect is occurring. Instead of rolling our own check if a path is a rect, since we have to replay the geometry anyway, the D2D path creates a Skia path and asks Skia if it's a rect. Ugly I know, but better than rolling our own IMHO.

For the test case "html test that reflects operations", I got a 2.25x speedup from ~2500-2700 ms to load the page to ~1100-1200ms. The original PDF also doesn't super hang the system (although this is a very fast machine).

Comment 27

[Mason Chang [Inactive] [:mchang]]

Just for reference, Chrome takes ~20 ms to render the test page, Edge takes ~70-80 ms.

Comment 28

[Bas Schouten (:bas.schouten)]

Comment on attachment 8689737 [details] [diff] [review]
Optimize push clip rects if the path is a rect

Review of attachment 8689737 [details] [diff] [review]:
-----------------------------------------------------------------

Let's do this, creating a cross-dependency between backends is a bad idea. Writing code to detect if a path is a rect isn't too complicated. If you're having trouble I'm fine with writing it.

Comment 29

[Bas Schouten (:bas.schouten)]

Comment on attachment 8689737 [details] [diff] [review]
Optimize push clip rects if the path is a rect

Review of attachment 8689737 [details] [diff] [review]:
-----------------------------------------------------------------

I meant let's not do this.

Comment 30

[Mason Chang [Inactive] [:mchang]]

(In reply to Bas Schouten (:bas.schouten) from comment #28)
> Comment on attachment 8689737 [details] [diff] [review]
> Optimize push clip rects if the path is a rect
> 
> Review of attachment 8689737 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let's do this, creating a cross-dependency between backends is a bad idea.
> Writing code to detect if a path is a rect isn't too complicated. If you're
> having trouble I'm fine with writing it.

Na thanks for the review. I wasn't sure if it was worth it to write our own. I'll do it.

Comment 31

[Mason Chang [Inactive] [:mchang]]

Attachment: Optimize push clip rects if the path is a rect

Wrote a simple checker to see if a path is a rect. It iterates through the lines of a d2d geometry and finds the top left point. From there, it detects the width/height of the rect, and checks to see if those points exists, and if so, it's a rect.

Comment 32

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8694970 [details] [diff] [review]
Optimize push clip rects if the path is a rect

Review of attachment 8694970 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/PathD2D.cpp
@@ +608,5 @@
> +    return mIsRect;
> +  }
> +
> +private:
> +  bool IsDiaganol(Point aFirstPoint, Point aSecondPoint)

IsDiagonal, I presume.

@@ +658,5 @@
> +bool
> +PathD2D::IsRect(Rect& aOutRect) const
> +{
> +  /***
> +   * D2D doesn't have an easy way to ask if a path is a rect.

This is meant to test for something that ends up looking like a rectangle, even if it's say, a 14 point path that ends up looking like a rect?

Comment 33

[Mason Chang [Inactive] [:mchang]]

(In reply to Milan Sreckovic [:milan] from comment #32)
> Comment on attachment 8694970 [details] [diff] [review]
> Optimize push clip rects if the path is a rect
> 
> Review of attachment 8694970 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/PathD2D.cpp
> @@ +608,5 @@
> > +    return mIsRect;
> > +  }
> > +
> > +private:
> > +  bool IsDiaganol(Point aFirstPoint, Point aSecondPoint)
> 
> IsDiagonal, I presume.
> 

Woops, thanks. Will fix this and include it with any of Bas' review.

> @@ +658,5 @@
> > +bool
> > +PathD2D::IsRect(Rect& aOutRect) const
> > +{
> > +  /***
> > +   * D2D doesn't have an easy way to ask if a path is a rect.
> 
> This is meant to test for something that ends up looking like a rectangle,
> even if it's say, a 14 point path that ends up looking like a rect?

This D2D version isn't, it only looks for simple rectangles that contain 4 points. I don't know what CG does to test if it's actually a rect or if it includes multiple point paths that are actually a rectangle.

Skia is a bit more lenient. If you have a segment, and two points are on the segment and continue in the same direction e.g. (5, 0) then (10, 0), that mid point can be considered part of a rectangle. But if the point goes in the opposite direction then continues in proper direction, e.g. (5, 0), (20, 0), (10, 0), then (25, 0), that would be an invalid rectangle.

Comment 34

[Bas Schouten (:bas.schouten)]

Comment on attachment 8694970 [details] [diff] [review]
Optimize push clip rects if the path is a rect

Review of attachment 8694970 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/2D.h
@@ +11,5 @@
>  #include "Rect.h"
>  #include "Matrix.h"
>  #include "Quaternion.h"
>  #include "UserData.h"
> +#include "nsDebug.h"

This is Moz2D, can't use nsDebug here.

@@ +587,5 @@
> +   * the input parameter will be filled with the proper rect.
> +   * Otherwise returns false and an empty rect;
> +   */
> +  virtual bool IsRect(Rect& aOutRect) const {
> +    NS_WARNING("Not yet implemented on this platform\n");

Use gfxWarning()

Comment 35

[Mason Chang [Inactive] [:mchang]]

Closing old bugs.

Comment 36

[Ed Morley [:emorley]]

This still reproduces.