765845 - Hang in DrawTargetD2D::FillGlyphs

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Whenever I use my laptop on battery Firefox seems to slow to a crawl. I am becoming more and more convinced that the slowdown I see is due to our 3d acceleration, probably because NVIDIA switches to a very slow low-power mode. Recently I started collecting minidumps whenever Firefox enters a hang state (where the window gets greyed out and the title bar says "Not Responding"). I collected about ten different dumps and each one had the following stack:

Main thread:
  DWrite.dll!operator!=()  + 0x19 bytes
  DWrite.dll!GlyphDataBlock<GlyphBitmapLayout,GlyphBitmapRasterizationParameters,GlyphBitmapRasterizationState>::IsEqualKeyImpl()  + 0x3a bytes
  DWrite.dll!CacheReader::LookupElement()  + 0xe6 bytes
  DWrite.dll!GlyphDataBlock<GlyphBitmapLayout,GlyphBitmapRasterizationParameters,GlyphBitmapRasterizationState>::FillFromReadOnlyCache()  + 0x76 bytes
  DWrite.dll!GlyphDataBlock<GlyphBitmapLayout,GlyphBitmapRasterizationParameters,GlyphBitmapRasterizationState>::ServerLookup()  + 0xa8 bytes
  DWrite.dll!GlyphDataBlock<GlyphBitmapLayout,GlyphBitmapRasterizationParameters,GlyphBitmapRasterizationState>::GetGlyphData()  + 0x57 bytes
  DWrite.dll!GetGlyphBitmaps()  + 0x93 bytes
  DWrite.dll!GlyphRunAnalysis::GlyphRunAnalysis()  + 0x2fc bytes
  DWrite.dll!DWriteFactory::CreateGlyphRunAnalysis()  + 0xfb bytes
  d2d1.dll!GlyphRunRealizer::Init()  + 0xf8 bytes
  d2d1.dll!CHwSurfaceRenderTarget::DrawGlyphRunInternal()  + 0xbc bytes
  d2d1.dll!CCommand_DrawGlyphRun::Execute()  + 0x27 bytes
  d2d1.dll!CHwSurfaceRenderTarget::ProcessBatch()  + 0x4c bytes
  d2d1.dll!CBatchSerializer::FlushInternal()  + 0x2e bytes
  d2d1.dll!CBatchSerializer::FlushInternalAndTryIncreaseStorage()  + 0x36 bytes
  d2d1.dll!CBatchSerializer::StartCommand<CCommand_SetTextRenderingParams>()  + 0xc bytes
  d2d1.dll!DrawingContext::SetTextRenderingParams()  + 0x34 bytes
  d2d1.dll!D2DRenderTargetBase<ID2D1HwndRenderTarget>::SetTextRenderingParams()  + 0x4e bytes
> gkmedias.dll!mozilla::gfx::DrawTargetD2D::FillGlyphs(mozilla::gfx::ScaledFont * aFont, const mozilla::gfx::GlyphBuffer & aBuffer, const mozilla::gfx::Pattern & aPattern, const mozilla::gfx::DrawOptions & aOptions, const mozilla::gfx::GlyphRenderingOptions * aRenderOptions)  Line 886
  xul.dll!gfxContext::AzureState::AzureState(const gfxContext::AzureState & __that)  + 0x80 bytes
  00388a04()
  000000aa()
  4a7faa24()

Is there anything that can be done about this?

Here are my graphics stats:

Adapter Description
  Intel(R) HD Graphics Family
Vendor ID
  0x8086
Device ID
  0x0126
Adapter RAM
  Unknown
Adapter Drivers
  igdumd64 igd10umd64 igd10umd64 igdumdx32 igd10umd32 igd10umd32
Driver Version
  8.15.10.2538
Driver Date
  9-26-2011
Adapter Description (GPU #2)
  NVIDIA Quadro 2000M
Vendor ID (GPU #2)
  0x10de
Device ID (GPU #2)
  0x0dda
Adapter RAM (GPU #2)
  2048
Adapter Drivers (GPU #2)
  nvd3dumx,nvwgf2umx,nvwgf2umx nvd3dum,nvwgf2um,nvwgf2um
Driver Version (GPU #2)
  8.17.12.7593
Driver Date (GPU #2)
  8-12-2011
Direct2D Enabled
  true
DirectWrite Enabled
  true (6.1.7601.17789)
ClearType Parameters
  ClearType parameters not found
WebGL Renderer
  Blocked for your graphics card because of unresolved driver issues.
GPU Accelerated Windows1/1
  Direct3D 10
AzureBackend
  direct2d

Comment 1

[Bas Schouten (:bas.schouten)]

Hrm, we will be calling this a little more with Azure content than without. Has this problem started recently? The fix should be easy. But I should fix this on aurora as well.

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Reduce calling frequency of SetTextRenderingParams

This makes us call TextRenderingParams as often as we do with Cairo. I never really knew this could cause a flush of the command stream so could be expensive.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Bas Schouten (:bas) from comment #1)
> Has this problem started recently?

Not sure about this particular stack, but FF has been really slow on battery power for a while now. I try to be plugged in whenever I can because of it so I just don't know.

Comment 4

[Alex Keybl [:akeybl]]

It's not clear that this is a recent regression, so no need to track for release. That being said, we would consider an uplift to Aurora 15 once fixed.

Comment 5

[Joe Drew (not getting mail)]

If this patch fixes the bug, then it's a regression from bug 715768, which landed for Firefox 15.

Ben, can you do a build with this patch in it?

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Wait, if this reduces the number of times we call this function that clearly causes an expensive blocking operation why wouldn't we take it?

I can do a build sometime soon, I guess.

Comment 7

[Joe Drew (not getting mail)]

Oh, we'd take it, but I'd like to know if it isn't fixed :)

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2dd31542fd6f

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oops, bugzilla changed the flags...

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

So... It seems that this changes rendering slightly. It's hard to see it, almost. I'll attach some screenshots.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 634260 [details] [diff] [review]
Reduce calling frequency of SetTextRenderingParams

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

::: gfx/2d/DrawTargetD2D.h
@@ +199,5 @@
>    mutable RefPtr<ID2D1RenderTarget> mRT;
>  
> +  // We store this to prevent excessive SetTextRenderingParams calls.
> +  RefPtr<IDWriteRenderingParams> mTextRenderingParams;
> +

This should say why we want to avoid excessive SetTextRenderingParams calls, but maybe that is better off at the SetTextRenderingParams call site.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Though it is also important to know why the rendering changed.

Comment 15

[Jonathan Kew [:jfkthame]]

It looks to me like there are two differences in the font rendering: different hinting behavior (maybe the difference between Natural and GDI-like rendering, for example?), and different gamma correction being used for the colored subpixel-AA pixels.

Comment 16

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #14)
> Though it is also important to know why the rendering changed.

I guess I could see this happening if it returned the TempRT for drawing somewhere... I'll add something for that to the patch.

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The verdict is in, though: this patch fixes the hang I was seeing. Now I see some slowdown on battery but all my profiles point to the layout engine.

Comment 18

[Bas Schouten (:bas.schouten)]

This is a regression then, let's fix it. I'll be uploading a new patch in a little bit.

Comment 19

[Bas Schouten (:bas.schouten)]

Attachment: Reduce calling frequency of SetTextRenderingParams v2

The previous patch was very broken (note how it sets mParams before actually checking if it's equal to params). This patch fixes all correctness issues.

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1e188186d0f8

Comment 21

[Bas Schouten (:bas.schouten)]

Comment on attachment 636098 [details] [diff] [review]
Reduce calling frequency of SetTextRenderingParams v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 715768
User impact if declined: Severe performance slowdown in some cases
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): Very low risk
String or UUID changes made by this patch: None

Comment 22

[Alex Keybl [:akeybl]]

Comment on attachment 636098 [details] [diff] [review]
Reduce calling frequency of SetTextRenderingParams v2

[Triage Comment]
New regression in FF15, with a low risk fix. Approved for Aurora 15.

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage Comment]
This missed landing in when 15 was on Aurora so now needs to land to beta.  Please update the status flags once this has landed.

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 636098 [details] [diff] [review]
Reduce calling frequency of SetTextRenderingParams v2

[Triage Comment]
bumping up the approval nomination accordingly.

Comment 25

[Joe Drew (not getting mail)]

http://hg.mozilla.org/releases/mozilla-beta/rev/1ac588e89dd5