1007897 - PDF.js spend 20-30% of time in SourceSurfaceCGBitmapContext::DrawTargetWillChange()

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

Looks like we're accidentally holding a snapshot to the canvas.

Comment 1

[Benoit Girard (:BenWa)]

mattwoodrow: BenWa: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/CopyableCanvasLayer.cpp#92
mattwoodrow: should set mSurface to nullptr after that CopySurface call

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch r=mattwoodrow on irc

With this patch it's down to 3%

Comment 3

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=69cb0df4c923

Comment 4

[Nicholas Nethercote [inactive]]

20--30% of what? I'm confused; the title sounds like this is a performance (speed) bug, but the patch looks like a MemShrink bug. Does freeing the memory also speed things up? Thanks.

Comment 5

[Andrew McCreight [:mccr8]]

There's a little more context on the Hacker News discussion that spawned this bug:
  https://news.ycombinator.com/item?id=7717191

"20% of the time is spent copying the canvas because someone is, likely erroneously, holding a reference to the canvas."

It just sounded to me like it had to be a memory win, too. ;)

Comment 6

[Benoit Girard (:BenWa)]

Sorry, I meant time. The problem is we hold a reference to an image longer then we need. When we go to touch the copy we see that someone is still referencing a snapshot of that image so we have to copy that image. The image will get deleted soon after but we still make a needless copy.

This will reduce peak memory but wont really decrease average memory usage.

Comment 7

[Nicholas Nethercote [inactive]]

> This will reduce peak memory but wont really decrease average memory usage.

Good! Given a choice, I prefer reducing peak than average.

Comment 8

[Andrew McCreight [:mccr8]]

Yeah, peak memory usage is a big problem on the phones, it can often spike quite high, and is hard to measure (and thus hard to fix).

Comment 9

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2eaf3d97610

Comment 10

[Benoit Girard (:BenWa)]

Unsurprisingly:
Improvement: Mozilla-Inbound - CanvasMark - MacOSX 10.6 (rev4) - 6.1% increase

Comment 11

[Andrew McCreight [:mccr8]]

Could the "<Regression> Mozilla-Inbound - CanvasMark - Android 2.2 (Native) - 9.12%" be from this?

Comment 12

[Benoit Girard (:BenWa)]

It's hard to convince how releasing an image (and a snapshot flag) earlier could cause a regression. But it's the most likely in the regression window.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d2eaf3d97610