Call HeapObjectHeader::checkHeader solely for its side-effect.

Call HeapObjectHeader::checkHeader solely for its side-effect.

This requires changing its signature. This is a preliminary stage to making it
private.

BUG=633030
Review-Url: https://codereview.chromium.org/2698673003
Cr-Commit-Position: refs/heads/master@{#460489}
Committed: https://chromium.googlesource.com/chromium/src/+/0749ec24fae74ec32d0567eef0e5ec43c84dbcb9

[palmer, 2017-02-16 00:53:24]

palmer@chromium.org changed reviewers:
+ haraken@chromium.org, sigbjornf@opera.se

[palmer, 2017-02-16 17:29:06]

The CQ bit was checked by palmer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 17:29:51]

Dry run: 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, 2017-02-16 18:15:02]

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

[commit-bot: I haz the power, 2017-02-16 18:15:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sof, 2017-02-16 18:50:04]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2017-02-16 18:50:04]

it looks like you haven't converted all occurrences of checkHeader() ?

(I don't understand how it can be turned into a private, but that can wait.)

https://codereview.chromium.org/2698673003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/HeapPage.h (right):

https://codereview.chromium.org/2698673003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/HeapPage.h:840: ASSERT(m_magic ==
getMagic());
ASSERT => DCHECK

[jbroman, 2017-02-16 19:08:10]

one drive-by comment (just saw "DCHECK" and "==" on the same line of code)

https://codereview.chromium.org/2698673003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/HeapPage.h (right):

https://codereview.chromium.org/2698673003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/HeapPage.h:840: ASSERT(m_magic ==
getMagic());
On 2017/02/16 at 18:50:04, sof wrote:
> ASSERT => DCHECK

DCHECK_EQ?

[palmer, 2017-02-16 19:45:19]

> it looks like you haven't converted all occurrences of checkHeader() ?

Fixed.

> (I don't understand how it can be turned into a private, but that can wait.)

Calls to checkHeader can just be in HOH's getters and setters; I don't think we
need any other call sites. (This may result in a net reduction of checkHeader
call sites, which would be nice.)

> ASSERT => DCHECK

I went with CHECK, so that it happens in all builds, which actually I intended
with the previous patch (!). And, CHECK_EQ, per jbroman.

[sof, 2017-02-16 19:59:44]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 20:00:50]

Dry run: 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, 2017-02-16 20:09:00]

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

[commit-bot: I haz the power, 2017-02-16 20:09:02]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sof, 2017-02-16 21:21:13]

On 2017/02/16 19:45:19, palmer wrote:
> > it looks like you haven't converted all occurrences of checkHeader() ?
> 
> Fixed.
> 
> > (I don't understand how it can be turned into a private, but that can wait.)
> 
> Calls to checkHeader can just be in HOH's getters and setters; I don't think
we
> need any other call sites. (This may result in a net reduction of checkHeader
> call sites, which would be nice.)
> 
> > ASSERT => DCHECK
> 
> I went with CHECK, so that it happens in all builds, which actually I intended
> with the previous patch (!). And, CHECK_EQ, per jbroman.

I'd like to see some perf numbers on that first, so go back to DCHECK(_EQ)()?

Do you need to rebase?

[palmer, 2017-02-16 21:59:45]

> I'd like to see some perf numbers on that first, so go back to DCHECK(_EQ)()?

OK; and after that I'll run them again with CHECK_EQ.

> Do you need to rebase?

Done.

[haraken, 2017-02-16 23:42:48]

In theory (=if the compiler is smart), this should not regress performance,
right?

LGTM if the perf numbers are good.

[palmer, 2017-02-17 00:37:13]

> In theory (=if the compiler is smart), this should not regress performance,
right?

I believe so, yes.

> LGTM if the perf numbers are good.

The numbers that have come in from the perf bots so far (not all in yet) seem
OK.

[sof, 2017-02-17 07:08:06]

On 2017/02/16 23:42:48, haraken wrote:
> In theory (=if the compiler is smart), this should not regress performance,
> right?
>

How do you reason? e.g., checking the mark bit (isMarked()) is now doing more
than just a bitop.

[palmer, 2017-02-28 00:11:43]

> > In theory (=if the compiler is smart), this should not regress performance,
> > right?
> 
> How do you reason? e.g., checking the mark bit (isMarked()) is now doing more
than just a bitop.

Not yet, in production builds at least — we're using DCHECK_EQ. However, we are
ultimately going to need to make it CHECK_EQ, so that we get the safety benefit
in production builds.

As for code size and speed, this CL should be a no-op: the same checks are done,
but the DCHECK_EQ is in the callee (checkHeader) instead of in every call site.
(Thus I think? there may even be a net reduction in object code size.) If the
callee compiles down to no code, as it currently should in production builds,
then the call sites can be eliminated too.

But ultimately, there is going to have to be code that does more than just
bitwise manipulation. It's going to be slightly slower. But, checkHeader and
getMagic are just about as small and as fast as it is possible for them to be, I
believe.

The other thing is that, once checkHeader is private, there will be some delta
in the number of call sites (possibly net neutral, if there are currently more
call sites than are necessary. But that's probably not the case.) When
checkHeader is private, we will have an easier job of determining that
checkHeader is called in all and only the necessary places. The number of call
sites might increase, but we'll be sure they are the minimum necessary for
correctness. Which is crucial here.

[palmer, 2017-02-28 00:11:52]

The CQ bit was checked by palmer@chromium.org

[palmer, 2017-02-28 00:11:55]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2698673003/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-02-28 00:12:31]

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, 2017-02-28 04:39:51]

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

[commit-bot: I haz the power, 2017-02-28 04:39:52]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[palmer, 2017-02-28 20:08:14]

The CQ bit was checked by palmer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 20:08:39]

Dry run: 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, 2017-03-01 00:05:20]

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

[commit-bot: I haz the power, 2017-03-01 00:05:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[palmer, 2017-03-28 01:33:48]

The CQ bit was checked by palmer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 01:34:31]

Dry run: 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, 2017-03-28 05:53:54]

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

[commit-bot: I haz the power, 2017-03-28 05:53:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[palmer, 2017-03-28 19:22:06]

> Try jobs failed on following builders:
>   linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

haraken: I can't seem to figure out why this CL explodes on ASan builds. As far
as I can tell, no memory accesses have changed. Do you have any clues?

[sof, 2017-03-28 19:43:23]

On 2017/03/28 19:22:06, palmer wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> haraken: I can't seem to figure out why this CL explodes on ASan builds. As
far
> as I can tell, no memory accesses have changed. Do you have any clues?

NO_SANITIZE_ADDRESS (and other NO_SANITIZEs) only applies to the entry point it
annotates, i.e., all memory accesses are exempted from ASan checks, but that
doesn't extend into the calls that the method body makes.

Hence you need to annotate Check<Op>Impl()s in logging.h to address this one w/
this CL, which I'm not sure we want to. It might be better to steer away from
using DCHECK_EQ() and have DCHECK() instead check a local 'bool' binding for the
condition.

[palmer, 2017-03-28 21:17:29]

> NO_SANITIZE_ADDRESS (and other NO_SANITIZEs) only applies to the entry point
it annotates, i.e., all memory accesses are exempted from ASan checks, but that
doesn't extend into the calls that the method body makes.
> 
> Hence you need to annotate Check<Op>Impl()s in logging.h to address this one
w/ this CL, which I'm not sure we want to. It might be better to steer away from
using DCHECK_EQ() and have DCHECK() instead check a local 'bool' binding for the
condition.

Cool, thanks. :) Let's see if it blends.

[palmer, 2017-03-28 21:17:51]

The CQ bit was checked by palmer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 21:18:03]

Dry run: 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, 2017-03-28 21:18:15]

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

[commit-bot: I haz the power, 2017-03-28 21:18:16]

Dry run: CQ has no permission to schedule in bucket
master.tryserver.chromium.linux

[palmer, 2017-03-28 21:20:26]

The CQ bit was checked by palmer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 21:20:35]

Dry run: 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, 2017-03-28 21:20:46]

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

[commit-bot: I haz the power, 2017-03-28 21:20:47]

Dry run: CQ has no permission to schedule in bucket
master.tryserver.chromium.linux

[palmer, 2017-03-28 22:04:01]

The CQ bit was checked by palmer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 22:04:24]

Dry run: 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, 2017-03-29 00:06:02]

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

[commit-bot: I haz the power, 2017-03-29 00:06:03]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[palmer, 2017-03-29 16:55:42]

The CQ bit was checked by palmer@chromium.org

[palmer, 2017-03-29 16:55:42]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2698673003/#ps100001
(title: "Use DCHECK on a bool instead of DCHECK_EQ.")

[commit-bot: I haz the power, 2017-03-29 16:56:20]

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, 2017-03-29 19:28:58]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490806542563820,
"parent_rev": "d49d131187ed4e0c3dd5124cdd6ba41cbc06a2a9", "commit_rev":
"0749ec24fae74ec32d0567eef0e5ec43c84dbcb9"}

[commit-bot: I haz the power, 2017-03-29 19:29:47]

Description was changed from

==========
Call HeapObjectHeader::checkHeader solely for its side-effect.

This requires changing its signature. This is a preliminary stage to making it
private.

BUG=633030
==========

to

==========
Call HeapObjectHeader::checkHeader solely for its side-effect.

This requires changing its signature. This is a preliminary stage to making it
private.

BUG=633030

Review-Url: https://codereview.chromium.org/2698673003
Cr-Commit-Position: refs/heads/master@{#460489}
Committed:
https://chromium.googlesource.com/chromium/src/+/0749ec24fae74ec32d0567eef0e5...
==========

[commit-bot: I haz the power, 2017-03-29 19:29:48]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/0749ec24fae74ec32d0567eef0e5...