Avoid unnecessary comparison if both Typeface are NULL

Avoid unnecessary comparison if both Typeface are NULL

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/48fc0ea272916a04f947bcd5bfce44fdab1f16b4

[Jin Yang, 2015-06-12 06:49:59]

jin.a.yang@intel.com changed reviewers:
+ bungeman@google.com, reed@google.com

[Jin Yang, 2015-06-12 06:51:26]

On 2015/06/12 06:49:59, Jin Yang wrote:
> mailto:jin.a.yang@intel.com changed reviewers:
> + mailto:bungeman@google.com, mailto:reed@google.com

PTAL, if both inputs are NULL, it will return directly. The patch imroves my
drawRect recording case by 10%.

[mtklein, 2015-06-12 11:58:13]

mtklein@google.com changed reviewers:
+ mtklein@google.com

[mtklein, 2015-06-12 11:58:14]

From a go-faster perspective, this CL makes sense and looks good to me. 
uniqueID() can be expensive, particularly the first time it's called on a
typeface.  We might want to leave a comment about this added new check being
there for performance.

I'm curious about where this shows up on a profile.  I'm only seeing a few call
sites for SkTypeface::Equal(), none of which should be involved in picture
recording.

Which of them are you measuring?  Is it the call in src/pipe/SkGPipeWrite.cpp? 
Can you post more details or a link to code to your benchmark?  I want to make
sure we can verify the effect and prevent regression in the future.

[bungeman-skia, 2015-06-12 14:05:24]

Note the (apparently only use outside Skia) in Blink at
Source/platform/fonts/FontPlatformData.cpp , probably for similar reasons. This
static method is pretty much a leftover and should be cleaned up anyway
(possibly removed). As a note here as to what is really happening, any NULL
passed in is getting resolved to a default typeface which may be slow
(especially the first time). I'm guessing that if your test called
SkTypeface::GetDefaultTypeface before starting the timer there would be a big
difference.

[Jin Yang, 2015-06-17 04:51:17]

On 2015/06/12 11:58:14, mtklein wrote:
> From a go-faster perspective, this CL makes sense and looks good to me. 
> uniqueID() can be expensive, particularly the first time it's called on a
> typeface.  We might want to leave a comment about this added new check being
> there for performance.
> 
> I'm curious about where this shows up on a profile.  I'm only seeing a few
call
> sites for SkTypeface::Equal(), none of which should be involved in picture
> recording.
> 
> Which of them are you measuring?  Is it the call in src/pipe/SkGPipeWrite.cpp?

> Can you post more details or a link to code to your benchmark?  I want to make
> sure we can verify the effect and prevent regression in the future.

Yes, exactly, it is called in SkGPipeWrite.cpp. My case is very simple, just to
drawRect thousands of times with a deferred canvas, so much recording is
involved.

[Jin Yang, 2015-06-17 04:53:39]

On 2015/06/12 14:05:24, bungeman1 wrote:
> Note the (apparently only use outside Skia) in Blink at
> Source/platform/fonts/FontPlatformData.cpp , probably for similar reasons.
This
> static method is pretty much a leftover and should be cleaned up anyway
> (possibly removed). As a note here as to what is really happening, any NULL
> passed in is getting resolved to a default typeface which may be slow
> (especially the first time). I'm guessing that if your test called
> SkTypeface::GetDefaultTypeface before starting the timer there would be a big
> difference.

It is calling drawRect many times with deferred canvas, every time record the
call, two NULL will be passed into this Equal function and trigger
GetDefaultTypeface, it is expensive, right?

[mtklein, 2015-06-17 11:41:50]

The CQ bit was checked by mtklein@google.com

[mtklein, 2015-06-17 11:41:51]

lgtm

Right, that makes sense to me.

[commit-bot: I haz the power, 2015-06-17 11:41:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177833004/1

[commit-bot: I haz the power, 2015-06-17 11:47:22]

Committed patchset #1 (id:1) as
https://skia.googlesource.com/skia/+/48fc0ea272916a04f947bcd5bfce44fdab1f16b4