Oilpan: Replace checkPointer() with ASSERT(checkPointer())

Oilpan: Replace checkPointer() with ASSERT(checkPointer())

It seems compilers are not always smart enough to ignore checkPointer().
This CL replaces checkPointer() with ASSERT(checkPointer()) to make
it more explicit that it is nop in production code.

BUG=420515

[haraken, 2015-06-30 05:45:09]

haraken@chromium.org changed reviewers:
+ keishi@chromium.org

[haraken, 2015-06-30 05:45:10]

PTAL

[keishi, 2015-06-30 05:45:59]

lgtm

[haraken, 2015-06-30 05:51:22]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2015-06-30 05:51:29]

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

[Yuta Kitamura, 2015-06-30 06:15:20]

yutak@chromium.org changed reviewers:
+ yutak@chromium.org

[Yuta Kitamura, 2015-06-30 06:15:21]

https://codereview.chromium.org/1208223003/diff/1/Source/platform/heap/Handle.h
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1208223003/diff/1/Source/platform/heap/Handle...
Source/platform/heap/Handle.h:327: }
IMO this use of ASSERT() is too confusing. When the checkPointer() fails,
the assertion failure occurs within two layers of ASSERT()s (one for
ASSERT(checkPointer()), another for the one within
Heap::isHeapObjectAlive()), which isn't a situation a normal developer
would expect.

I really would like to see some evidence (like disassembled code)
that the original code incurs some overhead after optimization.
I believe all modern compilers can optimize out functions like this.

If the overhead is inevitable, then why don't you use inline or
ALWAYS_INLINE, i.e.

    #if ENABLE(ASSERT)
        bool checkPointer() { ... }
    #else
        inline /* or ALWAYS_INLINE */ bool checkPointer() { }
    #endif

[Yuta Kitamura, 2015-06-30 06:15:59]

For the purpose of blocking CQ, not lgtm.

[haraken, 2015-06-30 06:20:53]

On 2015/06/30 06:15:21, Yuta Kitamura wrote:
>
https://codereview.chromium.org/1208223003/diff/1/Source/platform/heap/Handle.h
> File Source/platform/heap/Handle.h (right):
> 
>
https://codereview.chromium.org/1208223003/diff/1/Source/platform/heap/Handle...
> Source/platform/heap/Handle.h:327: }
> IMO this use of ASSERT() is too confusing. When the checkPointer() fails,
> the assertion failure occurs within two layers of ASSERT()s (one for
> ASSERT(checkPointer()), another for the one within
> Heap::isHeapObjectAlive()), which isn't a situation a normal developer
> would expect.
> 
> I really would like to see some evidence (like disassembled code)
> that the original code incurs some overhead after optimization.
> I believe all modern compilers can optimize out functions like this.
> 
> If the overhead is inevitable, then why don't you use inline or
> ALWAYS_INLINE, i.e.
> 
>     #if ENABLE(ASSERT)
>         bool checkPointer() { ... }
>     #else
>         inline /* or ALWAYS_INLINE */ bool checkPointer() { }
>     #endif

I don't have any evidence about checkPointer(). Your argument sounds reasonable,
dropping this CL.

While I was working on https://codereview.chromium.org/1213133002/,
state->checkThread() seems not omitted in non-assert builds in some cases, so I
wanted to avoid this kind of risk. In case of checkPointer(), I agree it must be
always inlined.

[commit-bot: I haz the power, 2015-06-30 06:32:01]

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

[commit-bot: I haz the power, 2015-06-30 06:32:02]

A disapproval has been posted by following reviewers: yutak@chromium.org.
Please make sure to get positive review before another CQ attempt.