1110580 - Bad performance with some PDF [slow canvas2d text drawing]

Status: REOPENED

(Core :: Graphics: Canvas2D, defect, P3)

Description

[khchanel]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141126041045

Steps to reproduce:

Browsing certain PDF - e.g. http://www.msy.com.au/Parts/PARTS.pdf
Using the built-in JS PDF viewer


Actual results:

PDF rendering is very slow, sluggish on loading, such as when scrolling to a new page, zooming in/out


Expected results:

smooth and fast

Comment 1

[Loic]

I guess PDF Viewer doesn't like these long tables.

Comment 2

[khchanel]

Attachment: Slow PDF Sample

Attaching a PDF sample as reference/backup

Comment 3

[Petruta Horea [:phorea]]

Renders fine using latest Nightly 38.0a1 2015-01-19 under Win 7 32-bit.
Could you try in newer Firefox versions? (http://nightly.mozilla.org/) Thank you!

Comment 4

[khchanel]

(In reply to Petruta Rasa [QA] [:petruta] from comment #3)
> Renders fine using latest Nightly 38.0a1 2015-01-19 under Win 7 32-bit.
> Could you try in newer Firefox versions? (http://nightly.mozilla.org/) Thank
> you!

Nope no observable improvements.

Note that the rendering is fine. but the bug report is about the performance being unsatisfying.
Comparing to chrome native pdf reader or adobe plugin.

Hint: just try zooming in/out in Firefox pdf reader vs Chrome pdf reader

Comment 5

[Petruta Horea [:phorea]]

Thanks, confirming for performance issues.

Comment 6

[Kenan]

It is not perfect (compared to chrome), but works for me Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:97.0) Gecko/20100101 Firefox/97.0 Build: 20211215215113

It got improved a bit with 2017-02-01 : (Try Scaling from 100% to 400% as a test)

pushlog_url: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9c06e744b1befb3a2e2fdac7414ce18220774a1d&tochange=f985243bb630b2c78cd57731c8d8ab191aa09527
Actually not pdf related bugs...

Comment 7

[Marco Castelluccio [:marco]]

Maybe bug 1334749 helped?

Comment 8

[Calixte Denizet (:calixte)]

I analyzed a profile and I don't see anything wrong with this pdf.
A lot of improvements have been made in pdf.js so this bug is likely outdated.
If it's still reproducible for someone, please reopen and provide a profile.

Comment 9

[Marco Castelluccio [:marco]]

On Linux it's still reproducible (though it's much better than it used to be): https://share.firefox.dev/3KZjr7i.

Comment 10

[Calixte Denizet (:calixte)]

With nightly on windows 11, 25% of the time is spent in fillText and close to 0% in save.
With nightly on a macbook pro m1, 40% in fillText and 6% in save.
And in :marco's profile, fillText and save have almost the same percentage and it doesn't make sense for me.

For info, save is called here:
https://github.com/mozilla/pdf.js/blob/32ae0e4867a3670aba529bb32496958cebe446f3/src/display/canvas.js#L2277

:nical, do you have an idea on what could be wrong ?

Comment 11

[Nicolas Silva [:nical]]

This profile https://share.firefox.dev/3MIY2Qh shows the C++ stuff inside fillText and save.

• For me most of the canvas rendering time is spent fillText only ~40% of which does in skia's glyph rendering. There is a lot of small stuff around involving memory allocations which perhaps we could optimize somehow, I'm not very familiar with the text rendering ins and outs. I've already come accross some complaints that the per-call overhead of fillText is significantly higher in firefox than in chrome (I don't have the bug numbers handy).
• save isn't necessarily expensive per se, but it appears to be the first canvas command in the frame so it triggers EnsureTarget which allocates backing storage for the canvas, clears it and copies content from the previous frame if need be. The clearing part is where the time is spent on my linux desktop apparently, probably mostly pagefaults on a freshly allocated shmem. It would be faster if the first canvas command was something that fills the entire canvas with an opaque pattern, in which case we are usually able to skip the copy. The clearing happens because we are using RGBX instead of RGBA which, with skia, forces us to initialize the alpha channel, see https://searchfox.org/mozilla-central/rev/a730b83206183bf097820c5780eef0d5e4103b9d/gfx/layers/client/TextureClient.cpp#1255. I wonder if we actually need this in all cases (if we are covering the canvas with opaque surface (again) we probably don't need the alpha channel to be initialized sine the command would ovewrite it. That said it's still an optimization that may not be applicable to what pdfjs does, and if the time is spent in pagefaults while accessing the shared memory for the first time, then a large part of the slowness will just move to whatever operation touches the pixels first. Also the overhead of EnsureTarget is likely going to be very different on windows where we currently use a different canvas backend and a different type of texture storage.

Lee, do you know off hands if some of the memory allocations in CanvasBidProcessor::FillText can be avoided or skipped, in particular there's a bunch of time spent in gfxFontGroup::MakeTextRun and gfxContext::CreatePreservingTransformOrNull?

Comment 12

[Lee Salzman [:lsalzman]]

Jonathan is probably more familiar with CanvasBidiProcessor...

Comment 13

[Thorin [:thorin]]

(In reply to Nicolas Silva [:nical] from comment #11)

I've already come accross some complaints that the per-call overhead of fillText is significantly higher in firefox than in chrome (I don't have the bug numbers handy).

Bug 1360222 , Bug 1651284 , Bug 601176 ?

Comment 14

[Jonathan Kew [:jfkthame]]

(In reply to Lee Salzman [:lsalzman] from comment #12)

Jonathan is probably more familiar with CanvasBidiProcessor...

One thing to note -- so we don't get sidetracked -- is that despite the name CanvasBidiProcessor in the stack here, it's not actually anything bidi-related that is taking the time (although if the content were mixed-direction, that might become a factor).

I think the major factor that's hurting us here is that pdf.js appears to issue a separate fillText call for each individual glyph, and there are an awful lot of glyphs on the page in this example. Every fillText operation pays the overhead of creating a gfxTextRun and initializing its glyph array, and then tears it down again. If we were making a single fillText call for an entire string (e.g. each cell of the tables), the overhead would be vastly reduced.

Because we're making these individual per-glyph fillText calls and creating single-glyph textruns, that means we also end up making a separate Skia call (SkCanvas::drawTextBlob) for every glyph, instead of passing glyphs in batches; I expect that ends up being a lot less efficient.

It may be that in some cases it's necessary to process each glyph individually when rendering a PDF, but in this case at least, I see that we're making whole-string measureText calls (I'm guessing these are related to setting up the invisible text layer that is used to support selection, etc), so I'm not sure why we can't do fillText with the same granularity, instead of per-glyph.

(Maybe we could come up with a stripped-down form of textRun that only supports single-character runs, and comes with reduced overhead, and use that to handle single-character fillText calls. Though in principle even a single-character string could have complex shaping behavior given the right OpenType font, so this may not be as simple as it sounds. And that wouldn't address the issue of the separate per-glyph calls to Skia for painting.)

Comment 15

[Jonas Jenwald [:Snuffleupagus]]

(In reply to Jonathan Kew (:jfkthame) from comment #14)

(In reply to Lee Salzman [:lsalzman] from comment #12)

Jonathan is probably more familiar with CanvasBidiProcessor...

I think the major factor that's hurting us here is that pdf.js appears to issue a separate fillText call for each individual glyph, and there are an awful lot of glyphs on the page in this example.

Please note that in PDF documents glyphs are, unfortunately, more often than not individually positioned. Hence it's most likely very difficult, in general, to coalesce fillText-calls without (badly) regressing text rendering in the process.[1]

I see that we're making whole-string measureText calls (I'm guessing these are related to setting up the invisible text layer that is used to support selection, etc), so I'm not sure why we can't do fillText with the same granularity, instead of per-glyph.

Please note that rendering and text-selection uses slightly different code-paths, even from a PDF specification perspective. Furthermore, for performance reasons, the (invisible) text-layer in PDF.js is by its design less exact than the actual rendering.

---

[1] There's even been attempts at doing this in the past, but those where quickly abandoned because it just didn't work out; see one example in https://github.com/mozilla/pdf.js/pull/8652

Comment 16

[Jonathan Kew [:jfkthame]]

I think there are a couple of things we can do relatively easily that should help a bit here.

(1) The CanvasRenderingContext2d::DrawOrMeasureText method processes the text twice via nsBidiPresUtils::ProcessText, because in general it needs the overall width of the string before it can correctly draw anything (in bidi cases, or for text alignment reasons, etc). This means we run the bidi algorithm and textrun creation (one for each sub-run of the text) twice, replacing CanvasBidiProcessor's "current text run" for each sub-run we handle.

However, in the (common) case of a simple, unidirectional string, there's only a single sub-run needing a textrun, so we can get away with just building the textrun once (at the measuring stage), and then re-use that same textrun for drawing.

(2) In nsBidiPresUtils::ProcessText, we analyze the string for direction runs and changes in the unicode bidi class, and split it into sub-runs that are each handled with a separate textrun. However, if the string is a single unicode character (as when pdf.js is drawing glyphs), we know there will only be one run, so we could use a simplified codepath for this case.

With these enhancements, I'm seeing the total time spent under fillText reduced from around 255ms to 210ms when jumping to the last page of the attached testcase; still not great, but a clear improvement.

I'm going to move this bug to Canvas2d, as the work proposed here is specifically about optimizing that API. Then if there are other pdf.js performance issues we'd like to try and address, let's spin those off to separate bugs.

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1110580 - patch 1 - Accelerate canvas2d.fillText for simple unidirectional strings by reusing the same textrun to draw as we created to measure the text. r=lsalzman

In general, the canvas text-drawing code makes two passes over the string, one to measure it and the second to draw.
Each time, it has to find individual direction runs and create a separate gfxTextRun for each.
However, in the (common) case of a simple unidirectional string, we're only going to have a single run,
so it's wasteful to construct it twice; the CanvasBidiProcessor can just re-use the run from the measurement
pass when it needs to draw.

In particular, this will apply to pdf.js drawing, where each glyph is handled in a separate fillText call.
Many other uses of canvas text should also benefit somewhat.

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1110580 - patch 2 - Provide a simplified codepath for nsBidiPresUtils::ProcessText in the case where it is handling a single Unicode character. r=lsalzman

Specifically, pdf.js does a lot of single-character fillText calls, which will hit this path.

Depends on D145426

Comment 19

[Jonathan Kew [:jfkthame]]

(In reply to Jonas Jenwald [:Snuffleupagus] from comment #15)

(In reply to Jonathan Kew (:jfkthame) from comment #14)

(In reply to Lee Salzman [:lsalzman] from comment #12)

Jonathan is probably more familiar with CanvasBidiProcessor...

I think the major factor that's hurting us here is that pdf.js appears to issue a separate fillText call for each individual glyph, and there are an awful lot of glyphs on the page in this example.

Please note that in PDF documents glyphs are, unfortunately, more often than not individually positioned. Hence it's most likely very difficult, in general, to coalesce fillText-calls without (badly) regressing text rendering in the process.[1]

Yes, I think you're right; since posting comment 14, I was thinking some more about this, and reaching that same conclusion. Though it does depend on the individual PDF and how it was generated; some documents do include longer strings e.g. with the Tj operator without applying per-glyph positioning. Would pdf.js map that to a single fillText call, or does it unconditionally "decompose" the string to individual characters?

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae257e919e51
patch 1 - Accelerate canvas2d.fillText for simple unidirectional strings by reusing the same textrun to draw as we created to measure the text. r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/d4adb945d17b
patch 2 - Provide a simplified codepath for nsBidiPresUtils::ProcessText in the case where it is handling a single Unicode character. r=lsalzman

Comment 21

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/ae257e919e51
https://hg.mozilla.org/mozilla-central/rev/d4adb945d17b

Comment 22

[Marco Castelluccio [:marco]]

The performance is still the same for me, most of the time being spent in "save".

The call stack is different between my profile (https://share.firefox.dev/3wb91v8) and yours (https://share.firefox.dev/3MIY2Qh).
In my case:
__memmove_avx_unaligned_erms
SkSpriteBlitter_Memcpy::blitRect(int, int, int, int)
SkScan::FillIRect(SkIRect const&, SkRegion const*, SkBlitter*)
SkScan::FillIRect(SkIRect const&, SkRasterClip const&, SkBlitter*)
SkDraw::drawBitmap(SkBitmap const&, SkMatrix const&, SkRect const*, SkPaint const&) const
SkBitmapDevice::drawBitmap(SkBitmap const&, SkMatrix const&, SkRect const*, SkPaint const&)
SkBitmapDevice::drawBitmapRect(SkBitmap const&, SkRect const*, SkRect const&, SkPaint const&, SkCanvas::SrcRectConstraint)
SkBaseDevice::drawImageRect(SkImage const*, SkRect const*, SkRect const&, SkPaint const&, SkCanvas::SrcRectConstraint)
SkCanvas::onDrawImage(SkImage const*, float, float, SkPaint const*)
SkCanvas::drawImage(SkImage const*, float, float, SkPaint const*)
mozilla::gfx::DrawTargetSkia::BlendSurface(mozilla::gfx::SourceSurface*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::CompositionOp)
mozilla::layers::TextureClient::CopyToTextureClient(mozilla::layers::TextureClient*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits> const*)
mozilla::layers::PersistentBufferProviderShared::BorrowDrawTarget(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&)
mozilla::dom::CanvasRenderingContext2D::EnsureTarget(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const*, bool)
mozilla::dom::CanvasRenderingContext2D::Save()
CanvasRenderingContext2D.save

In your case:
ARGBSetRow_X86
ARGBRect
mozilla::layers::ShmemTextureData::Create(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>, mozilla::gfx::SurfaceFormat, mozilla::gfx::BackendType, mozilla::layers::LayersBackend, mozilla::layers::TextureFlags, mozilla::layers::TextureAllocationFlags, mozilla::ipc::IShmemAllocator*)
mozilla::layers::TextureClient::CreateForRawBufferAccess(mozilla::layers::LayersIPCChannel*, mozilla::gfx::SurfaceFormat, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>, mozilla::gfx::BackendType, mozilla::layers::LayersBackend, mozilla::layers::TextureFlags, mozilla::layers::TextureAllocationFlags)
mozilla::layers::TextureClient::CreateForDrawing(mozilla::layers::TextureForwarder*, mozilla::gfx::SurfaceFormat, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>, mozilla::layers::KnowsCompositor*, mozilla::layers::BackendSelector, mozilla::layers::TextureFlags, mozilla::layers::TextureAllocationFlags)
mozilla::layers::TextureClient::CreateForDrawing(mozilla::layers::KnowsCompositor*, mozilla::gfx::SurfaceFormat, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>, mozilla::layers::BackendSelector, mozilla::layers::TextureFlags, mozilla::layers::TextureAllocationFlags)
mozilla::layers::PersistentBufferProviderShared::BorrowDrawTarget(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&)
mozilla::dom::CanvasRenderingContext2D::EnsureTarget(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const*, bool)
mozilla::dom::CanvasRenderingContext2D::Save()
CanvasRenderingContext2D.save

Comment 23

[Nicolas Silva [:nical]]

The call stack is a tad different but it's the same root cause overall, time is spent under EnsureTarget and instead of clearing the new canvas buffer, it's copying the previous frame into the new one where the time goes in your profile.

It is hard for us to optimize away this copy. It happens because the compositor is reading from the canvas while the content process wants to update the canvas for the new frame. Unless unless we can tell that the first drawing command is overwriting everything we have to do that copy. The time is spent in SkSpriteBlitter_Memcpy::blitRect which is does memcpy per row of pixels so we can't do much better than that.

It would be good to check that in this case we are still indeed reading the canvas on the compositor side. We should be able to release the canvas's buffer on the compositor side as soon as it has been uploaded to a texture, we have code to do that and it worked pretty well a while back but with this type of timing optimizations it's hard to write tests.

Looks like the performance are still not where we'd want them to be so reopenning.

Comment 24

[Nicolas Silva [:nical]]

It looks like when transitioning to webrender we lost part of the copy-on-write optimization with canvas buffers. It looks like we don't unlock the canvas's buffer as soon as its content has been uploaded to a texture which means we rarely get to reuse the previous frame's buffer.

Comment 25

[Marco Castelluccio [:marco]]

Another similar case in bug 810214.