Cache a screen compatible DC to avoid re-creating it excessively.

Cache a screen compatible DC to avoid re-creating it excessively. 

Profiling revealed that creating and deleting the DC carries 
a significant cost. 

This is an important optimization when you consider the case 
of ElideRectangleText(), which calls font.GetStringWidth() 
on many substrings of the input string. Each such call 
currently creates a temporary DC in RenderTextWin when using 
canvas_skia_skia.cc. 

Having a global compatible DC allows all those calls to 
re-use the same DC instead of creating and deleting them. 

This needs to be global because CanvasSkia::SizeStringInt() 
is a static function, so we can't just store it e.g. on the canvas. 

BUG=105550
TEST=CreateCompatibleDC() and DeleteDC() should disappear 
from the profiler output when running with use_canvas_skia_skia=1 
and rapidly clicking the SSL bubble on paypal.com

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120594

[Alexei Svitkine (slow), 2012-02-03 18:09:30]

Hey Scott,

Can you take a look at this? After the other performance CLs, this is the last
thing that's costing a lot in the new canvas_skia_skia.cc path on Windows.

[sky, 2012-02-03 18:34:06]

http://codereview.chromium.org/9323011/diff/2003/ui/gfx/screen.h
File ui/gfx/screen.h (right):

http://codereview.chromium.org/9323011/diff/2003/ui/gfx/screen.h#newcode68
ui/gfx/screen.h:68: #if defined(OS_WIN)
I don't like defining this here. Move it into it's own class/namespace.

http://codereview.chromium.org/9323011/diff/2003/ui/views/widget/native_widge...
File ui/views/widget/native_widget_win.cc (right):

http://codereview.chromium.org/9323011/diff/2003/ui/views/widget/native_widge...
ui/views/widget/native_widget_win.cc:1316: gfx::Screen::ReleaseCompatibleDC();
This isn't the right place for a release, and it doesn't cover win aura. Instead
create a singleton window that listens for this sort of thing. I want to say we
have such an object some where (ProcessSingleton is a bit similar), but I
couldn't readily locate it.

[Alexei Svitkine (slow), 2012-02-03 22:06:51]

Done. Can you take another look?

[sky, 2012-02-03 23:14:42]

http://codereview.chromium.org/9323011/diff/1006/ui/gfx/screen_compatible_dc_...
File ui/gfx/screen_compatible_dc_win.cc (right):

http://codereview.chromium.org/9323011/diff/1006/ui/gfx/screen_compatible_dc_...
ui/gfx/screen_compatible_dc_win.cc:43: LRESULT CALLBACK ListenerWindowProc(HWND
hwnd, UINT message,
nit: each param on its own line.

http://codereview.chromium.org/9323011/diff/1006/ui/gfx/screen_compatible_dc_...
ui/gfx/screen_compatible_dc_win.cc:95: DCHECK(listener_window_);
ui::CheckWindowCreated

http://codereview.chromium.org/9323011/diff/1006/ui/gfx/screen_compatible_dc_...
File ui/gfx/screen_compatible_dc_win.h (right):

http://codereview.chromium.org/9323011/diff/1006/ui/gfx/screen_compatible_dc_...
ui/gfx/screen_compatible_dc_win.h:22: operator HDC() { return hdc_; }
Is there a reason you like this better than a static getter?
We generally avoid operators like this.

http://codereview.chromium.org/9323011/diff/1006/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/9323011/diff/1006/ui/ui.gyp#newcode323
ui/ui.gyp:323: 'gfx/screen_compatible_dc_win.cc',
sort

[Alexei Svitkine (slow), 2012-02-06 14:37:38]

http://codereview.chromium.org/9323011/diff/1006/ui/gfx/screen_compatible_dc_...
File ui/gfx/screen_compatible_dc_win.cc (right):

http://codereview.chromium.org/9323011/diff/1006/ui/gfx/screen_compatible_dc_...
ui/gfx/screen_compatible_dc_win.cc:43: LRESULT CALLBACK ListenerWindowProc(HWND
hwnd, UINT message,
On 2012/02/03 23:14:42, sky wrote:
> nit: each param on its own line.

Done.

http://codereview.chromium.org/9323011/diff/1006/ui/gfx/screen_compatible_dc_...
ui/gfx/screen_compatible_dc_win.cc:95: DCHECK(listener_window_);
On 2012/02/03 23:14:42, sky wrote:
> ui::CheckWindowCreated

Done.

http://codereview.chromium.org/9323011/diff/1006/ui/gfx/screen_compatible_dc_...
File ui/gfx/screen_compatible_dc_win.h (right):

http://codereview.chromium.org/9323011/diff/1006/ui/gfx/screen_compatible_dc_...
ui/gfx/screen_compatible_dc_win.h:22: operator HDC() { return hdc_; }
On 2012/02/03 23:14:42, sky wrote:
> Is there a reason you like this better than a static getter?
> We generally avoid operators like this.

I based this off of base::win::ScopedCreateDC, which also has the HDC operator.
Let me know if you prefer me to change this to a HDC get() method instead.

I decided to make it a scoper because it will make it easy to add debugging code
to check for overlapping uses if we ever suspect someone is using this outside
the UI thread or in an overlapping fashion...

http://codereview.chromium.org/9323011/diff/1006/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/9323011/diff/1006/ui/ui.gyp#newcode323
ui/ui.gyp:323: 'gfx/screen_compatible_dc_win.cc',
On 2012/02/03 23:14:42, sky wrote:
> sort

Done.

[sky, 2012-02-06 16:43:08]

http://codereview.chromium.org/9323011/diff/1006/ui/gfx/screen_compatible_dc_...
File ui/gfx/screen_compatible_dc_win.h (right):

http://codereview.chromium.org/9323011/diff/1006/ui/gfx/screen_compatible_dc_...
ui/gfx/screen_compatible_dc_win.h:22: operator HDC() { return hdc_; }
On 2012/02/06 14:37:38, Alexei Svitkine wrote:
> On 2012/02/03 23:14:42, sky wrote:
> > Is there a reason you like this better than a static getter?
> > We generally avoid operators like this.
> 
> I based this off of base::win::ScopedCreateDC, which also has the HDC
operator.
> Let me know if you prefer me to change this to a HDC get() method instead.
> 
> I decided to make it a scoper because it will make it easy to add debugging
code
> to check for overlapping uses if we ever suspect someone is using this outside
> the UI thread or in an overlapping fashion...

The style guide recommends against it, so I say go with get.

[asvitkine_google, 2012-02-06 16:47:24]

> The style guide recommends against it, so I say go with get.

Done.

[sky, 2012-02-06 17:00:29]

LGTM

[commit-bot: I haz the power, 2012-02-06 17:33:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/9323011/9001

[commit-bot: I haz the power, 2012-02-06 20:13:37]

Change committed as 120594

[cpu_(ooo_6.6-7.5), 2012-02-08 20:50:18]

1- Are you sure message_only windows get the message you care about?

2- be careful not to use this outside your case, it is not listening for palette
changes or theme changes

3- For non-top level windows it is a posted message so I don't know how
effective this is going to be.

[Alexei Svitkine (slow), 2012-02-08 22:11:24]

+msw

Thanks for checking this over Carlos!

On Wed, Feb 8, 2012 at 3:50 PM,  <cpu@chromium.org> wrote:
> 1- Are you sure message_only windows get the message you care about?

You're right. It does seem that WM_DISPLAYCHANGE isn't being sent to
this hwnd. :(

(I tried changing the screen resolution to trigger it.)

> 2- be careful not to use this outside your case, it is not listening for
> palette changes or theme changes

Good point. Perhaps making this a helper class is the wrong approach
since it won't be applicable for other users - and also since it can't
even listen for WM_DISPLAYCHANGE.

I am also wondering if WM_DISPLAYCHANGE even matters for the use in
RenderTextWin - certainly we don't care about resolution change - and
I'd guess we don't care about bit depth either - since ultimately Skia
does the drawing...

Hmm... Maybe in this case it's better to just cache it as a static
member of RenderTextWin? I'm not sure in what kind of circumstances,
if any, we'd actually need to invalidate it - now that I think about
this further... Mike, any insight here?

> 3- For non-top level windows it is a posted message so I don't know how
> effective this is going to be.

[msw, 2012-02-09 01:49:41]

> Hmm... Maybe in this case it's better to just cache it as a static
> member of RenderTextWin? I'm not sure in what kind of circumstances,
> if any, we'd actually need to invalidate it - now that I think about
> this further... Mike, any insight here?

The MSDN docs for ScriptShape and ScriptPlace point to:
msdn.microsoft.com/en-us/library/windows/desktop/dd317726.aspx
"never pass the same handle for different fonts or different sizes."

A static RenderTextWin HDC member seems would apparently need invalidation
anytime we switch up the font or size. Perhaps we should maintain a limited map
of (font, size) -> cached HDCs? Or perhaps we could use a single static HDC for
the default font/size and use temporary local HDCs for other settings?

[Alexei Svitkine (slow), 2012-02-09 13:55:41]

On 2012/02/09 01:49:41, msw wrote:
> > Hmm... Maybe in this case it's better to just cache it as a static
> > member of RenderTextWin? I'm not sure in what kind of circumstances,
> > if any, we'd actually need to invalidate it - now that I think about
> > this further... Mike, any insight here?
> 
> The MSDN docs for ScriptShape and ScriptPlace point to:
> msdn.microsoft.com/en-us/library/windows/desktop/dd317726.aspx
> "never pass the same handle for different fonts or different sizes."

That's not an issue here, since the code calls SelectObject() with the hfont the
HDC prior to the ScriptShape() and ScriptPlace() calls.

[Alexei Svitkine (slow), 2012-02-09 13:56:14]

That's not an issue here, since the code calls SelectObject() with the hfont on
the
HDC prior to the ScriptShape() and ScriptPlace() calls.

[msw, 2012-02-09 19:05:35]

Then it might be okay to cache a single static HDC. Give it a quick test across
fonts/sizes/resolutions/bit-depths/dpis/etc. in Views examples.