check for wide-open inside quickContains

check for wide-open inside quickContains

Seems to shave off 1-2% from simplerects, but a little hard to tell for sure given its a slightly noisy gm. This is because the early-out allows us to skip creating the modified rect to check against (which includes 2 int->float steps).

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2258403002

[reed1, 2016-08-20 17:49:28]

Description was changed from

==========
check for wide-open inside quickContains

BUG=skia:
==========

to

==========
check for wide-open inside quickContains

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2258403002
==========

[reed1, 2016-08-20 17:50:10]

Description was changed from

==========
check for wide-open inside quickContains

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2258403002
==========

to

==========
check for wide-open inside quickContains

Seems to shave off 1-2% from simplerects, but a little hard to tell for sure
given its a slightly noisy gm

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2258403002
==========

[reed1, 2016-08-20 17:50:20]

reed@google.com changed reviewers:
+ bsalomon@google.com

[reed1, 2016-08-20 17:51:01]

Description was changed from

==========
check for wide-open inside quickContains

Seems to shave off 1-2% from simplerects, but a little hard to tell for sure
given its a slightly noisy gm

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2258403002
==========

to

==========
check for wide-open inside quickContains

Seems to shave off 1-2% from simplerects, but a little hard to tell for sure
given its a slightly noisy gm. This is because the early-out allows us to skip
creating the modified rect to check against (which includes 2 int->float steps).

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2258403002
==========

[reed1, 2016-08-20 17:51:07]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2016-08-20 17:51:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 13:01:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 13:01:07]

No LGTM from a valid reviewer yet. Please ask for an LGTM from a
full Skia committer

[bsalomon, 2016-08-22 13:35:03]

On 2016/08/22 13:01:07, commit-bot: I haz the power wrote:
> No LGTM from a valid reviewer yet. Please ask for an LGTM from a
> full Skia committer

I notice that isWideOpen is not defined inline. Maybe LTCG is inlining it but
probably worth defining it inline. Also wondering whether we should do something
like this:

SkClipStack.h:


class SkClipStack {
public:
    bool quickContains(...) const {
        if (this->isWideOpen()) {
            return true;
        }
        return this->internalQuickContains(...);
    }
private:
    bool internalQuickContains(...) const;
};

SkClipStack.cpp:

bool SkClipStack::internalQuickContains(...) const {
     // expensive stuff worth a function call.
}

Then all the current and future callers get the benefit of the inline fast out.

[reed1, 2016-08-22 16:12:47]

Are you suggesting to do that instead of this CL, or to also make isWideOpen
inlined?

[bsalomon, 2016-08-22 16:20:56]

On 2016/08/22 16:12:47, reed1 wrote:
> Are you suggesting to do that instead of this CL, or to also make isWideOpen
> inlined?

I was thinking both (inline isWideOpen() and make SkClipStack::quickContains()
have an inline fast path that relies on it).