Make base::WeakPtr::Get() fast

Make base::WeakPtr::Get() fast

It was previous calling the out-of-line method WeakReference::is_valid().
That means that code like

  if (my_weak_ptr)
    my_weak_ptr->foo();

would do two calls to is_valid(), plus tests and branching.

The is_valid() method showed up as one of the most frequently called non-inline
functions during browser start-up on Windows (about 1M calls).

is_valid() was also inefficient in itself, because it had to do a null-check
flag_, as well as checking if the flag was marked valid.

This patch removes the null-pointer check by using a sentinel Flag object instead of
a nullptr. And instead of storing a bool in the Flag, it stores a pointer-sized
bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently
setting EFLAGS so that the code above just becomes a few loads, AND, branch,
and call. (The size of Flag is unchanged; it grows into the padding only.)

This is expected to reduce the binary size by ~48KB on Android, and increase it
by 79KB on x64 Linux -- something that's paid for by the split-out refactorings
to WeakPtr and WeakPtrFactory.

Exposing the SequenceChecker calls in inline functions caused the
MessageLoopTestType*.Nesting test to overflow on Win64 dcheck release builds,
which is why this patch also lowers the recursion depth there.

BUG=728324, 738183, 738193
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2963623002
Cr-Commit-Position: refs/heads/master@{#487048}
Committed: https://chromium.googlesource.com/chromium/src/+/3856eab55bb241fb054a878d9922e65e7b5b0931

[hans, 2017-06-27 21:58:32]

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

[commit-bot: I haz the power, 2017-06-27 21:58:59]

Dry run: CQ is trying da patch.

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

[hans, 2017-06-27 22:00:42]

hans@chromium.org changed reviewers:
+ thakis@chromium.org

[hans, 2017-06-27 22:00:43]

Please take a look.

[commit-bot: I haz the power, 2017-06-27 22:01:45]

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

[commit-bot: I haz the power, 2017-06-27 22:01:46]

Dry run: Try jobs failed on following builders:
  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-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_...)

[Nico, 2017-06-27 22:05:41]

https://codereview.chromium.org/2963623002/diff/1/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (left):

https://codereview.chromium.org/2963623002/diff/1/base/memory/weak_ptr.h#oldc...
base/memory/weak_ptr.h:204: #if 0
This looks like your diffbase might be off?

[hans, 2017-06-27 22:09:50]

https://codereview.chromium.org/2963623002/diff/1/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (left):

https://codereview.chromium.org/2963623002/diff/1/base/memory/weak_ptr.h#oldc...
base/memory/weak_ptr.h:204: #if 0
On 2017/06/27 22:05:41, Nico (vacation Jun 30-Jul 11) wrote:
> This looks like your diffbase might be off?

Yeah, something is not right here :-(

[hans, 2017-06-27 22:13:52]

Here we go. Please take another look.

[hans, 2017-06-27 22:15:34]

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

[commit-bot: I haz the power, 2017-06-27 22:16:11]

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-06-27 22:28:27]

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

[commit-bot: I haz the power, 2017-06-27 22:28:28]

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

[hans, 2017-06-27 23:04:51]

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

[commit-bot: I haz the power, 2017-06-27 23:05:17]

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-06-27 23:34:57]

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

[commit-bot: I haz the power, 2017-06-27 23:35:00]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Nico, 2017-06-28 13:57:27]

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

[commit-bot: I haz the power, 2017-06-28 13:57:48]

Dry run: CQ is trying da patch.

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

[Nico, 2017-06-28 14:36:56]

Since this is a potentially subtle CL, I think it'd be cool if we could split as
much as possible into smaller preparatory CLs.

(General guideline I like: "a CL should change behavior or refactor, but not do
both")

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (right):

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.h#...
base/memory/weak_ptr.h:135: // takes up space in the class.
This bit seems unrelated to what's in the CL description, and is a space
optimization, not a time optimization. Can this be done in a separated CL, with
an estimate how much space it saves?

(Slightly related thread "[chromium-dev] PSA : Sequencification and PostTask
paradigm update", and fairly related thread "ThreadChecker vs. NonThreadSafe"
(also on chromium-dev))

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr_un...
File base/memory/weak_ptr_unittest.nc (right):

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr_un...
base/memory/weak_ptr_unittest.nc:20: #if defined(NCTEST_AUTO_DOWNCAST)  //
[r'fatal error: static_assert failed "other must be a subclass of T"']
The changes in this CL also feel like they could be in a different CL (plus the
static_asserts in the header)

[commit-bot: I haz the power, 2017-06-28 14:53:16]

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

[commit-bot: I haz the power, 2017-06-28 14:53:17]

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

[Nico, 2017-06-28 14:54:10]

Super cool, lgtm, but I would've probably turned this into like 5 CLs :-)

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.cc
File base/memory/weak_ptr.cc (right):

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.cc...
base/memory/weak_ptr.cc:25: // null flag doesn't participate in the sequence
checks.
Maybe add ", see DCHECK in Invalidate()"

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.cc...
base/memory/weak_ptr.cc:27: AddRef();  // Stayin' alive.
Remove comment, software is serious business. (Or make the comment answer "why?"
https://github.com/berkerpeksag/notes/blob/master/books/code-complete.md#chap...
etc)

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.cc...
base/memory/weak_ptr.cc:31: ANNOTATE_SCOPED_MEMORY_LEAK;
I don't think this is needed, lsan doesn't report things reachable from globals
as leaks.

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.cc...
base/memory/weak_ptr.cc:32: static Flag* g_null_flag = new Flag(kNullFlagTag);
If you wanted you could use CR_DEFINE_STATIC_LOCAL.

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (right):

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.h#...
base/memory/weak_ptr.h:98: static Flag* nullFlag();
google (and chromium) style spells functions NullFlag() (or, optionally,
null_flag() if it's inline and cheap)

Give this function a comment. (Is it valid to call methods on the flag; what's
the NullFlag() good for, can I addref() it, does its refcount ever become small
enough for this presumably singleton object to be deleted, etc)

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.h#...
base/memory/weak_ptr.h:103: return;
This looks kind of confusing – do you not want to set is_valid_ to 0 if this ==
nullFlag(), but only in dcheck builds? Probably fine either way since it's 0
already there so, setting it might be cheaper than the check. I think this is
correct, but it's not obvious with a casual glance. Maybe

#if DCHECK_IS_ON()
  if (this != NullFlag()) {
    // The flag being...
    DCHECK...
  }
#endif
  is_valid = 0

?

(Also, moving the functions from cc to h could be its own CL too...)

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.h#...
base/memory/weak_ptr.h:112: uintptr_t IsValid() const {
Add comment that this returns either all 0s or all 1s and can be used as a
bitmask.

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.h#...
base/memory/weak_ptr.h:193: uintptr_t ptr_;
Moving this to the base class also feels kind of unrelated to the main thrust
(but it's mentioned in the CL description)

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.h#...
base/memory/weak_ptr.h:263: T* get() const { return
reinterpret_cast<T*>(ref_.is_valid() & ptr_); }
Since this is so unusual, I'd probably add `// Intentionally bitwise and, see
comment on Ref::is_valid()

[Nico, 2017-06-28 14:54:57]

(and
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
looks like it might be genuinely unhappy about this change)

[hans, 2017-06-28 16:37:16]

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

[commit-bot: I haz the power, 2017-06-28 16:37:32]

Dry run: CQ is trying da patch.

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

[hans, 2017-06-28 16:38:25]

Thanks for the review!

On 2017/06/28 14:54:57, Nico (vacation Jun 30-Jul 11) wrote:
> (and
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
> looks like it might be genuinely unhappy about this change)

Yeah, trying to fix this now.

I'll split this into a couple of CLs and go over your comments once it's all
working correctly.

[commit-bot: I haz the power, 2017-06-28 17:18:46]

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

[commit-bot: I haz the power, 2017-06-28 17:18:47]

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

[hans, 2017-06-28 18:41:54]

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

[commit-bot: I haz the power, 2017-06-28 18:42:27]

Dry run: CQ is trying da patch.

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

[hans, 2017-06-28 18:44:49]

Description was changed from

==========
Make base::WeakPtr::Get() fast

It was previously calling WeakReference::is_valid() which was an out-of-line
method. That means that code like

  if (my_weak_ptr)
    my_weak_ptr->foo();

would do two calls to is_valid(), plus tests and branching.

The is_valid() method showed up as one of the most frequently called non-inline
functions during browser start-up on Windows (about 1M calls).

is_valid() was also inefficient because it had to do a null-pointer check on
flag_, as well as checking if the flag was marked valid.

This patch removes the null-pointer check by using a sentinel object instead of
a nullptr. And instead of storing a bool in the flag, it stores a pointer-sized
bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently
setting EFLAGS so that the code above just becomes a few loads, AND, branch,
and call. (The size of Flag is unchanged; it grows into the padding only.)

The patch also moves the constructors of WeakPtr and WeakPtrFactory
out-of-line. They were relying on the out-of-line constructor for WeakReference
anyway, so inlining didn't make sense. Since WeakPtr and WeakPtrFactory are
templates, the outlining is done by pushing a member to a non-template base
class.

This reduces binary size on Android by 64 KB and grows Linux with 27 KB.

BUG=728324
==========

to

==========
Make base::WeakPtr::Get() fast

It was previously calling WeakReference::is_valid() which was an out-of-line
method. That means code like

  if (my_weak_ptr)
    my_weak_ptr->foo();

would do two calls to is_valid(), plus tests and branching.

The is_valid() method showed up as one of the most frequently called non-inline
functions during browser start-up on Windows (about 1M calls).

is_valid() was also inefficient because it had to do a null-pointer check on
flag_, as well as checking if the flag was marked valid.

This patch removes the null-pointer check by using a sentinel object instead of
a nullptr. And instead of storing a bool in the flag, it stores a pointer-sized
bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently
setting EFLAGS so that the code above just becomes a few loads, AND, branch,
and call. (The size of Flag is unchanged; it grows into the padding only.)

BUG=728324
==========

[commit-bot: I haz the power, 2017-06-28 19:45:12]

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

[commit-bot: I haz the power, 2017-06-28 19:45:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hans, 2017-06-28 19:47:59]

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

[commit-bot: I haz the power, 2017-06-28 19:48:19]

Dry run: CQ is trying da patch.

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

[hans, 2017-06-28 20:37:35]

Addressed comments and split out sub-changes to
https://codereview.chromium.org/2961083002/ and
https://codereview.chromium.org/2959203002/

The first of those was also the one that broke headless_browsertests so
splitting was definitely the right idea :-)

Do you want to take another look or does the lgtm stand?

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.cc
File base/memory/weak_ptr.cc (right):

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.cc...
base/memory/weak_ptr.cc:25: // null flag doesn't participate in the sequence
checks.
On 2017/06/28 14:54:09, Nico (vacation Jun 30-Jul 11) wrote:
> Maybe add ", see DCHECK in Invalidate()"

Done.

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.cc...
base/memory/weak_ptr.cc:27: AddRef();  // Stayin' alive.
On 2017/06/28 14:54:09, Nico (vacation Jun 30-Jul 11) wrote:
> Remove comment, software is serious business. (Or make the comment answer
"why?"
>
https://github.com/berkerpeksag/notes/blob/master/books/code-complete.md#chap...
> etc)

Done.

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.cc...
base/memory/weak_ptr.cc:31: ANNOTATE_SCOPED_MEMORY_LEAK;
On 2017/06/28 14:54:09, Nico (vacation Jun 30-Jul 11) wrote:
> I don't think this is needed, lsan doesn't report things reachable from
globals
> as leaks.

Done.

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.cc...
base/memory/weak_ptr.cc:32: static Flag* g_null_flag = new Flag(kNullFlagTag);
On 2017/06/28 14:54:09, Nico (vacation Jun 30-Jul 11) wrote:
> If you wanted you could use CR_DEFINE_STATIC_LOCAL.

I don't think that would provide much value really?

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (right):

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.h#...
base/memory/weak_ptr.h:98: static Flag* nullFlag();
On 2017/06/28 14:54:10, Nico (vacation Jun 30-Jul 11) wrote:
> google (and chromium) style spells functions NullFlag() (or, optionally,
> null_flag() if it's inline and cheap)
> 
> Give this function a comment. (Is it valid to call methods on the flag; what's
> the NullFlag() good for, can I addref() it, does its refcount ever become
small
> enough for this presumably singleton object to be deleted, etc)

Done.

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.h#...
base/memory/weak_ptr.h:103: return;
On 2017/06/28 14:54:09, Nico (vacation Jun 30-Jul 11) wrote:
> This looks kind of confusing – do you not want to set is_valid_ to 0 if this
==
> nullFlag(), but only in dcheck builds? Probably fine either way since it's 0
> already there so, setting it might be cheaper than the check. I think this is
> correct, but it's not obvious with a casual glance. Maybe
> 
> #if DCHECK_IS_ON()
>   if (this != NullFlag()) {
>     // The flag being...
>     DCHECK...
>   }
> #endif
>   is_valid = 0
> 
> ?
> 
> (Also, moving the functions from cc to h could be its own CL too...)

I've tried to make this clearer with a comment. The point of the code is to skip
the sequence check for the Null Flag. I've tried to make this clearer.

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.h#...
base/memory/weak_ptr.h:112: uintptr_t IsValid() const {
On 2017/06/28 14:54:10, Nico (vacation Jun 30-Jul 11) wrote:
> Add comment that this returns either all 0s or all 1s and can be used as a
> bitmask.

Done.

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.h#...
base/memory/weak_ptr.h:135: // takes up space in the class.
On 2017/06/28 14:36:56, Nico (vacation Jun 30-Jul 11) wrote:
> This bit seems unrelated to what's in the CL description, and is a space
> optimization, not a time optimization. Can this be done in a separated CL,
with
> an estimate how much space it saves?

It's related in that it keeps the size from growing with my change.

On trunk, Flag has an 8-byte ref count + 1-byte bool + 1-byte empty Sequence
Checker. With 8-byte alignment that means its sizeof is 16.

With my change, we get 8-byte ref count + 8-byte uintptr_t + 1-byte empty
Sequence Checker. With 8-byte alignment, the sizeof would become 24.

So I'm removing the Sequence Checker in release builds to maintain Flag's size
at 16 bytes.

> (Slightly related thread "[chromium-dev] PSA : Sequencification and PostTask
> paradigm update", and fairly related thread "ThreadChecker vs. NonThreadSafe"
> (also on chromium-dev))

I saw them, but is there any action I need to take in this CL?

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.h#...
base/memory/weak_ptr.h:193: uintptr_t ptr_;
On 2017/06/28 14:54:10, Nico (vacation Jun 30-Jul 11) wrote:
> Moving this to the base class also feels kind of unrelated to the main thrust
> (but it's mentioned in the CL description)

Done. Splitting this out.

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.h#...
base/memory/weak_ptr.h:263: T* get() const { return
reinterpret_cast<T*>(ref_.is_valid() & ptr_); }
On 2017/06/28 14:54:09, Nico (vacation Jun 30-Jul 11) wrote:
> Since this is so unusual, I'd probably add `// Intentionally bitwise and, see
> comment on Ref::is_valid()

Done.

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr_un...
File base/memory/weak_ptr_unittest.nc (right):

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr_un...
base/memory/weak_ptr_unittest.nc:20: #if defined(NCTEST_AUTO_DOWNCAST)  //
[r'fatal error: static_assert failed "other must be a subclass of T"']
On 2017/06/28 14:36:56, Nico (vacation Jun 30-Jul 11) wrote:
> The changes in this CL also feel like they could be in a different CL (plus
the
> static_asserts in the header)

Yeah, this is part of the "move ptr_ into WeakPtrBase" change; splitting that
out.

[Nico, 2017-06-28 20:57:03]

still lgtm

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (right):

https://codereview.chromium.org/2963623002/diff/40001/base/memory/weak_ptr.h#...
base/memory/weak_ptr.h:135: // takes up space in the class.
Ah ok, sounds good then

https://codereview.chromium.org/2963623002/diff/100001/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (right):

https://codereview.chromium.org/2963623002/diff/100001/base/memory/weak_ptr.h...
base/memory/weak_ptr.h:101: // consturcted or because they have been
invalidated. This can be used like
typo consturcted

[hans, 2017-06-28 21:08:36]

Description was changed from

==========
Make base::WeakPtr::Get() fast

It was previously calling WeakReference::is_valid() which was an out-of-line
method. That means code like

  if (my_weak_ptr)
    my_weak_ptr->foo();

would do two calls to is_valid(), plus tests and branching.

The is_valid() method showed up as one of the most frequently called non-inline
functions during browser start-up on Windows (about 1M calls).

is_valid() was also inefficient because it had to do a null-pointer check on
flag_, as well as checking if the flag was marked valid.

This patch removes the null-pointer check by using a sentinel object instead of
a nullptr. And instead of storing a bool in the flag, it stores a pointer-sized
bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently
setting EFLAGS so that the code above just becomes a few loads, AND, branch,
and call. (The size of Flag is unchanged; it grows into the padding only.)

BUG=728324
==========

to

==========
Make base::WeakPtr::Get() fast

It was previously calling WeakReference::is_valid() which was an out-of-line
method. That means code like

  if (my_weak_ptr)
    my_weak_ptr->foo();

would do two calls to is_valid(), plus tests and branching.

The is_valid() method showed up as one of the most frequently called non-inline
functions during browser start-up on Windows (about 1M calls).

is_valid() was also inefficient because it had to do a null-pointer check on
flag_, as well as checking if the flag was marked valid.

This patch removes the null-pointer check by using a sentinel object instead of
a nullptr. And instead of storing a bool in the flag, it stores a pointer-sized
bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently
setting EFLAGS so that the code above just becomes a few loads, AND, branch,
and call. (The size of Flag is unchanged; it grows into the padding only.)

This is expected to reduce the binary size by ~48KB on Android, and increase it
by 79KB on x64 Linux -- something that's paid for by the split-out refactorings
to WeakPtr and WeakPtrFactory.

BUG=728324
==========

[hans, 2017-06-28 21:16:59]

Thanks!

https://codereview.chromium.org/2963623002/diff/100001/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (right):

https://codereview.chromium.org/2963623002/diff/100001/base/memory/weak_ptr.h...
base/memory/weak_ptr.h:101: // consturcted or because they have been
invalidated. This can be used like
On 2017/06/28 20:57:03, Nico (vacation Jun 30-Jul 11) wrote:
> typo consturcted

Done.

[hans, 2017-06-28 21:17:03]

The CQ bit was checked by hans@chromium.org

[hans, 2017-06-28 21:17:04]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2963623002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2017-06-28 21:17:34]

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-06-28 23:25:52]

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

[commit-bot: I haz the power, 2017-06-28 23:25:53]

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_...)

[hans, 2017-06-28 23:40:44]

The CQ bit was checked by hans@chromium.org

[hans, 2017-06-28 23:40:45]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2963623002/#ps160001
(title: "put the leak annotation back; fix media_unittests for some reason")

[commit-bot: I haz the power, 2017-06-28 23:41:12]

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-06-29 01:15:35]

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

[commit-bot: I haz the power, 2017-06-29 01:15:37]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Nico, 2017-06-29 10:02:52]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2017-06-29 10:02:59]

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-06-29 12:22:51]

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

[commit-bot: I haz the power, 2017-06-29 12:22:53]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Nico, 2017-06-29 12:29:10]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2017-06-29 12:29:27]

CQ is trying da patch.

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

[Nico, 2017-06-29 12:32:23]

On 2017/06/29 12:22:53, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

That bot's been consistently failing the same three tests in base. Maybe it's
something real.

[hans, 2017-06-29 15:04:24]

On 2017/06/29 12:32:23, Nico (vacation Jun 30-Jul 11) wrote:
> On 2017/06/29 12:22:53, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
> 
> That bot's been consistently failing the same three tests in base. Maybe it's
> something real.

I overlooked it due to all the other stuff. Investigating now.

[commit-bot: I haz the power, 2017-06-29 15:12:56]

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

[commit-bot: I haz the power, 2017-06-29 15:12:58]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hans, 2017-06-29 16:23:13]

On 2017/06/29 15:04:24, hans wrote:
> On 2017/06/29 12:32:23, Nico (vacation Jun 30-Jul 11) wrote:
> > On 2017/06/29 12:22:53, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
> > 
> > That bot's been consistently failing the same three tests in base. Maybe
it's
> > something real.
> 
> I overlooked it due to all the other stuff. Investigating now.

It's a stack overflow in MessageLoopTestType*.Nesting. That test runs 100 nested
tasks (a task that posts a task that ...) and with my patch in this build mode
it overflows 78 tasks deep (with MSVC only; not Clang).

Without my patch it overflows at 322 tasks deep. It seems my patch is growing
the stack frames, due to inlining I suppose.

I've changed the test depth to 50 instead.

[hans, 2017-06-29 16:23:50]

The CQ bit was checked by hans@chromium.org

[hans, 2017-06-29 16:23:50]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2963623002/#ps180001
(title: "Fix MessageLoopTest.Nesting stack overflow on win_chromium_x64_rel_ng")

[commit-bot: I haz the power, 2017-06-29 16:24:01]

CQ is trying da patch.

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

[hans, 2017-06-29 16:46:39]

On 2017/06/29 16:23:13, hans wrote:
> Without my patch it overflows at 322 tasks deep. It seems my patch is growing
> the stack frames, due to inlining I suppose.

Yes, it's the inlining of the SequenceChecker calls that does it. In a
non-dcheck build we can still run ~320 tasks deep with my patch.

[Nico, 2017-06-29 16:52:49]

If it's just that one test collection, and only in debug builds due to
sequencechecker, that seems ok, still lgtm. Maybe mention this in the CL
description too.

[hans, 2017-06-29 16:56:15]

Description was changed from

==========
Make base::WeakPtr::Get() fast

It was previously calling WeakReference::is_valid() which was an out-of-line
method. That means code like

  if (my_weak_ptr)
    my_weak_ptr->foo();

would do two calls to is_valid(), plus tests and branching.

The is_valid() method showed up as one of the most frequently called non-inline
functions during browser start-up on Windows (about 1M calls).

is_valid() was also inefficient because it had to do a null-pointer check on
flag_, as well as checking if the flag was marked valid.

This patch removes the null-pointer check by using a sentinel object instead of
a nullptr. And instead of storing a bool in the flag, it stores a pointer-sized
bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently
setting EFLAGS so that the code above just becomes a few loads, AND, branch,
and call. (The size of Flag is unchanged; it grows into the padding only.)

This is expected to reduce the binary size by ~48KB on Android, and increase it
by 79KB on x64 Linux -- something that's paid for by the split-out refactorings
to WeakPtr and WeakPtrFactory.

BUG=728324
==========

to

==========
Make base::WeakPtr::Get() fast

It was previously calling WeakReference::is_valid() which was an out-of-line
method. That means code like

  if (my_weak_ptr)
    my_weak_ptr->foo();

would do two calls to is_valid(), plus tests and branching.

The is_valid() method showed up as one of the most frequently called non-inline
functions during browser start-up on Windows (about 1M calls).

is_valid() was also inefficient because it had to do a null-pointer check on
flag_, as well as checking if the flag was marked valid.

This patch removes the null-pointer check by using a sentinel object instead of
a nullptr. And instead of storing a bool in the flag, it stores a pointer-sized
bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently
setting EFLAGS so that the code above just becomes a few loads, AND, branch,
and call. (The size of Flag is unchanged; it grows into the padding only.)

This is expected to reduce the binary size by ~48KB on Android, and increase it
by 79KB on x64 Linux -- something that's paid for by the split-out refactorings
to WeakPtr and WeakPtrFactory.

Exposing the SequenceChecker calls in inline functions caused the
MessageLoopTestType*.Nesting test to overflow on Win64 dcheck release builds,
which is why this patch also lowers the recursion depth there.

BUG=728324
==========

[hans, 2017-06-29 16:57:09]

On 2017/06/29 16:52:49, Nico (vacation Jun 30-Jul 11) wrote:
> If it's just that one test collection, and only in debug builds due to
> sequencechecker, that seems ok, still lgtm. Maybe mention this in the CL
> description too.

Not debug builds, but release builds with dchecks. I've updated the description.

[commit-bot: I haz the power, 2017-06-29 18:22:38]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1498753430170260,
"parent_rev": "ef23a3a329bc08acb698f1295fb050371be08b23", "commit_rev":
"526f714c9ae172c3b16581b7e11eb035fd14274e"}

[commit-bot: I haz the power, 2017-06-29 18:22:55]

Description was changed from

==========
Make base::WeakPtr::Get() fast

It was previously calling WeakReference::is_valid() which was an out-of-line
method. That means code like

  if (my_weak_ptr)
    my_weak_ptr->foo();

would do two calls to is_valid(), plus tests and branching.

The is_valid() method showed up as one of the most frequently called non-inline
functions during browser start-up on Windows (about 1M calls).

is_valid() was also inefficient because it had to do a null-pointer check on
flag_, as well as checking if the flag was marked valid.

This patch removes the null-pointer check by using a sentinel object instead of
a nullptr. And instead of storing a bool in the flag, it stores a pointer-sized
bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently
setting EFLAGS so that the code above just becomes a few loads, AND, branch,
and call. (The size of Flag is unchanged; it grows into the padding only.)

This is expected to reduce the binary size by ~48KB on Android, and increase it
by 79KB on x64 Linux -- something that's paid for by the split-out refactorings
to WeakPtr and WeakPtrFactory.

Exposing the SequenceChecker calls in inline functions caused the
MessageLoopTestType*.Nesting test to overflow on Win64 dcheck release builds,
which is why this patch also lowers the recursion depth there.

BUG=728324
==========

to

==========
Make base::WeakPtr::Get() fast

It was previously calling WeakReference::is_valid() which was an out-of-line
method. That means code like

  if (my_weak_ptr)
    my_weak_ptr->foo();

would do two calls to is_valid(), plus tests and branching.

The is_valid() method showed up as one of the most frequently called non-inline
functions during browser start-up on Windows (about 1M calls).

is_valid() was also inefficient because it had to do a null-pointer check on
flag_, as well as checking if the flag was marked valid.

This patch removes the null-pointer check by using a sentinel object instead of
a nullptr. And instead of storing a bool in the flag, it stores a pointer-sized
bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently
setting EFLAGS so that the code above just becomes a few loads, AND, branch,
and call. (The size of Flag is unchanged; it grows into the padding only.)

This is expected to reduce the binary size by ~48KB on Android, and increase it
by 79KB on x64 Linux -- something that's paid for by the split-out refactorings
to WeakPtr and WeakPtrFactory.

Exposing the SequenceChecker calls in inline functions caused the
MessageLoopTestType*.Nesting test to overflow on Win64 dcheck release builds,
which is why this patch also lowers the recursion depth there.

BUG=728324

Review-Url: https://codereview.chromium.org/2963623002
Cr-Commit-Position: refs/heads/master@{#483427}
Committed:
https://chromium.googlesource.com/chromium/src/+/526f714c9ae172c3b16581b7e11e...
==========

[commit-bot: I haz the power, 2017-06-29 18:22:57]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/526f714c9ae172c3b16581b7e11e...

[mgersh, 2017-06-29 21:23:51]

On 2017/06/29 18:22:57, commit-bot: I haz the power wrote:
> Committed patchset #10 (id:180001) as
>
https://chromium.googlesource.com/chromium/src/+/526f714c9ae172c3b16581b7e11e...

A lot of Cronet bots have tests failing and I suspect it might be caused by this
CL.

Example failure:
https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Bui...

Stack trace:
Stack Trace:
  RELADDR   FUNCTION                                                            
                                                                                
                                                                                
                                               FILE:LINE
  0005e97b  logging::LogMessage::~LogMessage()+110                              
                                                                                
                                                                                
                                              
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/logging.cc:553
  0002d0dd  base::internal::WeakReference::Flag::IsValid() const+64             
                                                                                
                                                                                
                                              
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/memory/weak_ptr.h:132
  0002d055  base::WeakPtr<JsonPrefStore>::get() const+8                         
                                                                                
                                                                                
                                              
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/memory/weak_ptr.h:287
  v------>  base::WeakPtr<net::URLRequestJob>::operator bool() const            
                                                                                
                                                                                
                                              
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/memory/weak_ptr.h:305
  00289155  void base::internal::InvokeHelper<true, void>::MakeItSo<void
(net::URLRequestJob::* const&)(bool, int), base::WeakPtr<net::URLRequestJob>
const&, bool const&, int>(void (net::URLRequestJob::* const&)(bool, int),
base::WeakPtr<net::URLRequestJob> const&, bool const&, int&&)+22 
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/bind_internal.h:292
  000da4e1  base::Callback<void (net::Error), (base::internal::CopyMode)0,
(base::internal::RepeatMode)0>::Run(net::Error) &&+28                           
                                                                                
                                                    
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/callback.h:91
  002e322d  net::URLRequestJob::ReadRawDataComplete(int)+248                    
                                                                                
                                                                                
                                              
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/net/url_request/url_request_job.cc:586
  0004237f  base::Callback<void (), (base::internal::CopyMode)0,
(base::internal::RepeatMode)0>::Run() &&+22                                     
                                                                                
                                                              
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/callback.h:91
  0004c359  base::debug::TaskAnnotator::RunTask(char const*,
base::PendingTask*)+140                                                         
                                                                                
                                                                  
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/debug/task_annotator.cc:59
  000655f1  base::MessageLoop::RunTask(base::PendingTask*)+180                  
                                                                                
                                                                                
                                              
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/message_loop/message_loop.cc:422
  00065851  base::MessageLoop::DeferOrRunPendingTask(base::PendingTask)+44      
                                                                                
                                                                                
                                              
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/message_loop/message_loop.cc:433
  00065a03  base::MessageLoop::DoWork()+166                                     
                                                                                
                                                                                
                                              
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/message_loop/message_loop.cc:540
  0006794b  base::MessagePumpLibevent::Run(base::MessagePump::Delegate*)+50     
                                                                                
                                                                                
                                              
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/message_loop/message_pump_libevent.cc:219
  00065435  base::MessageLoop::Run()+60                                         
                                                                                
                                                                                
                                              
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/message_loop/message_loop.cc:369
  0007ee61  base::RunLoop::Run()+100                                            
                                                                                
                                                                                
                                              
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/run_loop.cc:111
  000a02b5  base::Thread::Run(base::RunLoop*)+100                               
                                                                                
                                                                                
                                              
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/threading/thread.cc:255
  000a0595  base::Thread::ThreadMain()+432                                      
                                                                                
                                                                                
                                              
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/threading/thread.cc:338
  0009b633  base::(anonymous namespace)::ThreadFunc(void*)+66                   
                                                                                
                                                                                
                                              
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/threading/platform_thread_posix.cc:71
  0000d173  <unknown>                                                           
                                                                                
                                                                                
                                               /system/lib/libc.so
  0000d30b  <unknown>

[maniscalco, 2017-06-29 22:02:34]

Also, TSan failures:
https://bugs.chromium.org/p/chromium/issues/detail?id=738193

I'll see about reverting.

On 2017/06/29 21:23:51, mgersh wrote:
> On 2017/06/29 18:22:57, commit-bot: I haz the power wrote:
> > Committed patchset #10 (id:180001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/526f714c9ae172c3b16581b7e11e...
> 
> A lot of Cronet bots have tests failing and I suspect it might be caused by
this
> CL.
> 
> Example failure:
>
https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Bui...
> 
> Stack trace:
> Stack Trace:
>   RELADDR   FUNCTION                                                          
 
>                                                                               
 
>                                                                               
 
>                                                FILE:LINE
>   0005e97b  logging::LogMessage::~LogMessage()+110                            
 
>                                                                               
 
>                                                                               
 
>                                               
> /b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/logging.cc:553
>   0002d0dd  base::internal::WeakReference::Flag::IsValid() const+64           
 
>                                                                               
 
>                                                                               
 
>                                               
>
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/memory/weak_ptr.h:132
>   0002d055  base::WeakPtr<JsonPrefStore>::get() const+8                       
 
>                                                                               
 
>                                                                               
 
>                                               
>
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/memory/weak_ptr.h:287
>   v------>  base::WeakPtr<net::URLRequestJob>::operator bool() const          
 
>                                                                               
 
>                                                                               
 
>                                               
>
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/memory/weak_ptr.h:305
>   00289155  void base::internal::InvokeHelper<true, void>::MakeItSo<void
> (net::URLRequestJob::* const&)(bool, int), base::WeakPtr<net::URLRequestJob>
> const&, bool const&, int>(void (net::URLRequestJob::* const&)(bool, int),
> base::WeakPtr<net::URLRequestJob> const&, bool const&, int&&)+22 
> /b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/bind_internal.h:292
>   000da4e1  base::Callback<void (net::Error), (base::internal::CopyMode)0,
> (base::internal::RepeatMode)0>::Run(net::Error) &&+28                         
 
>                                                                               
 
>                                                     
> /b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/callback.h:91
>   002e322d  net::URLRequestJob::ReadRawDataComplete(int)+248                  
 
>                                                                               
 
>                                                                               
 
>                                               
>
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/net/url_request/url_request_job.cc:586
>   0004237f  base::Callback<void (), (base::internal::CopyMode)0,
> (base::internal::RepeatMode)0>::Run() &&+22                                   
 
>                                                                               
 
>                                                               
> /b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/callback.h:91
>   0004c359  base::debug::TaskAnnotator::RunTask(char const*,
> base::PendingTask*)+140                                                       
 
>                                                                               
 
>                                                                   
>
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/debug/task_annotator.cc:59
>   000655f1  base::MessageLoop::RunTask(base::PendingTask*)+180                
 
>                                                                               
 
>                                                                               
 
>                                               
>
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/message_loop/message_loop.cc:422
>   00065851  base::MessageLoop::DeferOrRunPendingTask(base::PendingTask)+44    
 
>                                                                               
 
>                                                                               
 
>                                               
>
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/message_loop/message_loop.cc:433
>   00065a03  base::MessageLoop::DoWork()+166                                   
 
>                                                                               
 
>                                                                               
 
>                                               
>
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/message_loop/message_loop.cc:540
>   0006794b  base::MessagePumpLibevent::Run(base::MessagePump::Delegate*)+50   
 
>                                                                               
 
>                                                                               
 
>                                               
>
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/message_loop/message_pump_libevent.cc:219
>   00065435  base::MessageLoop::Run()+60                                       
 
>                                                                               
 
>                                                                               
 
>                                               
>
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/message_loop/message_loop.cc:369
>   0007ee61  base::RunLoop::Run()+100                                          
 
>                                                                               
 
>                                                                               
 
>                                               
> /b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/run_loop.cc:111
>   000a02b5  base::Thread::Run(base::RunLoop*)+100                             
 
>                                                                               
 
>                                                                               
 
>                                               
>
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/threading/thread.cc:255
>   000a0595  base::Thread::ThreadMain()+432                                    
 
>                                                                               
 
>                                                                               
 
>                                               
>
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/threading/thread.cc:338
>   0009b633  base::(anonymous namespace)::ThreadFunc(void*)+66                 
 
>                                                                               
 
>                                                                               
 
>                                               
>
/b/build/slave/Android_Cronet_Builder__dbg_/build/src/base/threading/platform_thread_posix.cc:71
>   0000d173  <unknown>                                                         
 
>                                                                               
 
>                                                                               
 
>                                                /system/lib/libc.so
>   0000d30b  <unknown>

[hans, 2017-06-29 22:02:48]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2966633002/ by hans@chromium.org.

The reason for reverting is: This seems to have broken Cronet tests and Linux
TSan. See bugs..

[maniscalco, 2017-06-29 22:03:47]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2969513003/ by maniscalco@chromium.org.

The reason for reverting is: Suspect as cause of several Linux TSan test
failures:

https://bugs.chromium.org/p/chromium/issues/detail?id=738193
.

[hans, 2017-07-17 08:54:04]

Description was changed from

==========
Make base::WeakPtr::Get() fast

It was previously calling WeakReference::is_valid() which was an out-of-line
method. That means code like

  if (my_weak_ptr)
    my_weak_ptr->foo();

would do two calls to is_valid(), plus tests and branching.

The is_valid() method showed up as one of the most frequently called non-inline
functions during browser start-up on Windows (about 1M calls).

is_valid() was also inefficient because it had to do a null-pointer check on
flag_, as well as checking if the flag was marked valid.

This patch removes the null-pointer check by using a sentinel object instead of
a nullptr. And instead of storing a bool in the flag, it stores a pointer-sized
bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently
setting EFLAGS so that the code above just becomes a few loads, AND, branch,
and call. (The size of Flag is unchanged; it grows into the padding only.)

This is expected to reduce the binary size by ~48KB on Android, and increase it
by 79KB on x64 Linux -- something that's paid for by the split-out refactorings
to WeakPtr and WeakPtrFactory.

Exposing the SequenceChecker calls in inline functions caused the
MessageLoopTestType*.Nesting test to overflow on Win64 dcheck release builds,
which is why this patch also lowers the recursion depth there.

BUG=728324

Review-Url: https://codereview.chromium.org/2963623002
Cr-Commit-Position: refs/heads/master@{#483427}
Committed:
https://chromium.googlesource.com/chromium/src/+/526f714c9ae172c3b16581b7e11e...
==========

to

==========
Make base::WeakPtr::Get() fast

It was previously calling WeakReference::is_valid() which was an out-of-line
method. That means code like

  if (my_weak_ptr)
    my_weak_ptr->foo();

would do two calls to is_valid(), plus tests and branching.

The is_valid() method showed up as one of the most frequently called non-inline
functions during browser start-up on Windows (about 1M calls).

is_valid() was also inefficient because it had to do a null-pointer check on
flag_, as well as checking if the flag was marked valid.

This patch removes the null-pointer check by using a sentinel object instead of
a nullptr. And instead of storing a bool in the flag, it stores a pointer-sized
bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently
setting EFLAGS so that the code above just becomes a few loads, AND, branch,
and call. (The size of Flag is unchanged; it grows into the padding only.)

This is expected to reduce the binary size by ~48KB on Android, and increase it
by 79KB on x64 Linux -- something that's paid for by the split-out refactorings
to WeakPtr and WeakPtrFactory.

Exposing the SequenceChecker calls in inline functions caused the
MessageLoopTestType*.Nesting test to overflow on Win64 dcheck release builds,
which is why this patch also lowers the recursion depth there.

BUG=728324
==========

[hans, 2017-07-17 09:16:22]

Description was changed from

==========
Make base::WeakPtr::Get() fast

It was previously calling WeakReference::is_valid() which was an out-of-line
method. That means code like

  if (my_weak_ptr)
    my_weak_ptr->foo();

would do two calls to is_valid(), plus tests and branching.

The is_valid() method showed up as one of the most frequently called non-inline
functions during browser start-up on Windows (about 1M calls).

is_valid() was also inefficient because it had to do a null-pointer check on
flag_, as well as checking if the flag was marked valid.

This patch removes the null-pointer check by using a sentinel object instead of
a nullptr. And instead of storing a bool in the flag, it stores a pointer-sized
bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently
setting EFLAGS so that the code above just becomes a few loads, AND, branch,
and call. (The size of Flag is unchanged; it grows into the padding only.)

This is expected to reduce the binary size by ~48KB on Android, and increase it
by 79KB on x64 Linux -- something that's paid for by the split-out refactorings
to WeakPtr and WeakPtrFactory.

Exposing the SequenceChecker calls in inline functions caused the
MessageLoopTestType*.Nesting test to overflow on Win64 dcheck release builds,
which is why this patch also lowers the recursion depth there.

BUG=728324
==========

to

==========
Make base::WeakPtr::Get() fast

It was previous calling the out-of-line method WeakReference::is_valid().
That means that code like

  if (my_weak_ptr)
    my_weak_ptr->foo();

would do two calls to is_valid(), plus tests and branching.

The is_valid() method showed up as one of the most frequently called non-inline
functions during browser start-up on Windows (about 1M calls).

is_valid() was also inefficient in itself, because it had to do a null-check
flag_, as well as checking if the flag was marked valid.

This patch removes the null-pointer check by using a sentinel Flag object
instead of
a nullptr. And instead of storing a bool in the Flag, it stores a pointer-sized
bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently
setting EFLAGS so that the code above just becomes a few loads, AND, branch,
and call. (The size of Flag is unchanged; it grows into the padding only.)

This is expected to reduce the binary size by ~48KB on Android, and increase it
by 79KB on x64 Linux -- something that's paid for by the split-out refactorings
to WeakPtr and WeakPtrFactory.

Exposing the SequenceChecker calls in inline functions caused the
MessageLoopTestType*.Nesting test to overflow on Win64 dcheck release builds,
which is why this patch also lowers the recursion depth there.

BUG=728324,738183,738193
==========

[hans, 2017-07-17 09:20:37]

The cronet test has been disabled
(https://bugs.chromium.org/p/chromium/issues/detail?id=738183#c45) and I've
addressed the TSan error in the latest version of the patch, so I think this is
ready for re-landing.

[hans, 2017-07-17 09:20:53]

Description was changed from

==========
Make base::WeakPtr::Get() fast

It was previous calling the out-of-line method WeakReference::is_valid().
That means that code like

  if (my_weak_ptr)
    my_weak_ptr->foo();

would do two calls to is_valid(), plus tests and branching.

The is_valid() method showed up as one of the most frequently called non-inline
functions during browser start-up on Windows (about 1M calls).

is_valid() was also inefficient in itself, because it had to do a null-check
flag_, as well as checking if the flag was marked valid.

This patch removes the null-pointer check by using a sentinel Flag object
instead of
a nullptr. And instead of storing a bool in the Flag, it stores a pointer-sized
bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently
setting EFLAGS so that the code above just becomes a few loads, AND, branch,
and call. (The size of Flag is unchanged; it grows into the padding only.)

This is expected to reduce the binary size by ~48KB on Android, and increase it
by 79KB on x64 Linux -- something that's paid for by the split-out refactorings
to WeakPtr and WeakPtrFactory.

Exposing the SequenceChecker calls in inline functions caused the
MessageLoopTestType*.Nesting test to overflow on Win64 dcheck release builds,
which is why this patch also lowers the recursion depth there.

BUG=728324,738183,738193
==========

to

==========
Make base::WeakPtr::Get() fast

It was previous calling the out-of-line method WeakReference::is_valid().
That means that code like

  if (my_weak_ptr)
    my_weak_ptr->foo();

would do two calls to is_valid(), plus tests and branching.

The is_valid() method showed up as one of the most frequently called non-inline
functions during browser start-up on Windows (about 1M calls).

is_valid() was also inefficient in itself, because it had to do a null-check
flag_, as well as checking if the flag was marked valid.

This patch removes the null-pointer check by using a sentinel Flag object
instead of
a nullptr. And instead of storing a bool in the Flag, it stores a pointer-sized
bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently
setting EFLAGS so that the code above just becomes a few loads, AND, branch,
and call. (The size of Flag is unchanged; it grows into the padding only.)

This is expected to reduce the binary size by ~48KB on Android, and increase it
by 79KB on x64 Linux -- something that's paid for by the split-out refactorings
to WeakPtr and WeakPtrFactory.

Exposing the SequenceChecker calls in inline functions caused the
MessageLoopTestType*.Nesting test to overflow on Win64 dcheck release builds,
which is why this patch also lowers the recursion depth there.

BUG=728324,738183,738193
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[hans, 2017-07-17 09:22:03]

The CQ bit was checked by hans@chromium.org

[hans, 2017-07-17 09:22:03]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2963623002/#ps240001
(title: "Move WeakReference::Flag::Invalidate out-of-line; it's only called from
WeakReferenceOwner::Invalid…")

[commit-bot: I haz the power, 2017-07-17 09:22:10]

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-07-17 12:03:24]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1500283323186320,
"parent_rev": "2ae2828e20bfca183dcc7ea0070be9372c2cd07c", "commit_rev":
"3856eab55bb241fb054a878d9922e65e7b5b0931"}

[commit-bot: I haz the power, 2017-07-17 12:03:44]

Description was changed from

==========
Make base::WeakPtr::Get() fast

It was previous calling the out-of-line method WeakReference::is_valid().
That means that code like

  if (my_weak_ptr)
    my_weak_ptr->foo();

would do two calls to is_valid(), plus tests and branching.

The is_valid() method showed up as one of the most frequently called non-inline
functions during browser start-up on Windows (about 1M calls).

is_valid() was also inefficient in itself, because it had to do a null-check
flag_, as well as checking if the flag was marked valid.

This patch removes the null-pointer check by using a sentinel Flag object
instead of
a nullptr. And instead of storing a bool in the Flag, it stores a pointer-sized
bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently
setting EFLAGS so that the code above just becomes a few loads, AND, branch,
and call. (The size of Flag is unchanged; it grows into the padding only.)

This is expected to reduce the binary size by ~48KB on Android, and increase it
by 79KB on x64 Linux -- something that's paid for by the split-out refactorings
to WeakPtr and WeakPtrFactory.

Exposing the SequenceChecker calls in inline functions caused the
MessageLoopTestType*.Nesting test to overflow on Win64 dcheck release builds,
which is why this patch also lowers the recursion depth there.

BUG=728324,738183,738193
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Make base::WeakPtr::Get() fast

It was previous calling the out-of-line method WeakReference::is_valid().
That means that code like

  if (my_weak_ptr)
    my_weak_ptr->foo();

would do two calls to is_valid(), plus tests and branching.

The is_valid() method showed up as one of the most frequently called non-inline
functions during browser start-up on Windows (about 1M calls).

is_valid() was also inefficient in itself, because it had to do a null-check
flag_, as well as checking if the flag was marked valid.

This patch removes the null-pointer check by using a sentinel Flag object
instead of
a nullptr. And instead of storing a bool in the Flag, it stores a pointer-sized
bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently
setting EFLAGS so that the code above just becomes a few loads, AND, branch,
and call. (The size of Flag is unchanged; it grows into the padding only.)

This is expected to reduce the binary size by ~48KB on Android, and increase it
by 79KB on x64 Linux -- something that's paid for by the split-out refactorings
to WeakPtr and WeakPtrFactory.

Exposing the SequenceChecker calls in inline functions caused the
MessageLoopTestType*.Nesting test to overflow on Win64 dcheck release builds,
which is why this patch also lowers the recursion depth there.

BUG=728324,738183,738193
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2963623002
Cr-Commit-Position: refs/heads/master@{#487048}
Committed:
https://chromium.googlesource.com/chromium/src/+/3856eab55bb241fb054a878d9922...
==========

[commit-bot: I haz the power, 2017-07-17 12:03:45]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/3856eab55bb241fb054a878d9922...

[gab, 2017-07-18 20:31:07]

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

[gab, 2017-07-18 20:31:08]

Just saw this in weak_ptr.h and was curious so I came here :), drive-by lgtm w/
nits.

Too bad that this will make https://chromium-review.googlesource.com/c/527413/
(a wish to make validity checks thread-safe to support thread-safe cancelled
task pruning) harder... I guess we could have the uintptr_t use atomics but I'm
no longer convinced that would set EFLAGS..?

https://codereview.chromium.org/2963623002/diff/240001/base/memory/weak_ptr.cc
File base/memory/weak_ptr.cc (right):

https://codereview.chromium.org/2963623002/diff/240001/base/memory/weak_ptr.c...
base/memory/weak_ptr.cc:12: static constexpr uintptr_t kTrueMask =
~static_cast<uintptr_t>(0);
rm "static": it's redundant at file-scope level

https://codereview.chromium.org/2963623002/diff/240001/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (right):

https://codereview.chromium.org/2963623002/diff/240001/base/memory/weak_ptr.h...
base/memory/weak_ptr.h:135: SequenceChecker sequence_checker_;
You can use SEQUENCE_CHECKER() and friends macros to get a SequenceChecker
behind DCHECK_IS_ON() in one line. This is now the default recommendation and
wouldn't even require a comment :
https://cs.chromium.org/chromium/src/base/sequence_checker.h?type=cs&q=%23def...

[hans, 2017-07-19 08:08:23]

Thanks! Following up in https://chromium-review.googlesource.com/577399

https://codereview.chromium.org/2963623002/diff/240001/base/memory/weak_ptr.cc
File base/memory/weak_ptr.cc (right):

https://codereview.chromium.org/2963623002/diff/240001/base/memory/weak_ptr.c...
base/memory/weak_ptr.cc:12: static constexpr uintptr_t kTrueMask =
~static_cast<uintptr_t>(0);
On 2017/07/18 20:31:08, gab wrote:
> rm "static": it's redundant at file-scope level

Done.

https://codereview.chromium.org/2963623002/diff/240001/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (right):

https://codereview.chromium.org/2963623002/diff/240001/base/memory/weak_ptr.h...
base/memory/weak_ptr.h:135: SequenceChecker sequence_checker_;
On 2017/07/18 20:31:08, gab wrote:
> You can use SEQUENCE_CHECKER() and friends macros to get a SequenceChecker
> behind DCHECK_IS_ON() in one line. This is now the default recommendation and
> wouldn't even require a comment :
>
https://cs.chromium.org/chromium/src/base/sequence_checker.h?type=cs&q=%23def...

Done.