Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1173)

Issue 2963623002: Make base::WeakPtr::Get() fast (Closed)

Created:
5 months, 3 weeks ago by hans
Modified:
5 months ago
Reviewers:
gab, Nico
CC:
chromium-reviews, danakj+watch_chromium.org, gavinp+memory_chromium.org, vmpstr+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 2

Patch Set 2 : try again #

Patch Set 3 : static_assert in the converting copy ctor and update no-compile test #

Total comments: 23

Patch Set 4 : fix HasRefs; make linux_chromium_headless_rel happier? #

Patch Set 5 : split out outlining changes #

Patch Set 6 : address review comments #

Total comments: 2

Patch Set 7 : fix typo #

Patch Set 8 : rebase #

Patch Set 9 : put the leak annotation back; fix media_unittests for some reason #

Patch Set 10 : Fix MessageLoopTest.Nesting stack overflow on win_chromium_x64_rel_ng #

Patch Set 11 : rebase #

Patch Set 12 : don't invalidate the null flag; fix tsan error #

Patch Set 13 : Move WeakReference::Flag::Invalidate out-of-line; it's only called from WeakReferenceOwner::Invalid… #

Total comments: 4

Messages

Total messages: 88 (52 generated)
hans
Please take a look.
5 months, 3 weeks ago (2017-06-27 22:00:43 UTC) #4
Nico
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#oldcode204 base/memory/weak_ptr.h:204: #if 0 This looks like your diffbase might be ...
5 months, 3 weeks ago (2017-06-27 22:05:41 UTC) #7
hans
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#oldcode204 base/memory/weak_ptr.h:204: #if 0 On 2017/06/27 22:05:41, Nico (vacation Jun 30-Jul ...
5 months, 3 weeks ago (2017-06-27 22:09:50 UTC) #8
hans
Here we go. Please take another look.
5 months, 3 weeks ago (2017-06-27 22:13:52 UTC) #9
Nico
Since this is a potentially subtle CL, I think it'd be cool if we could ...
5 months, 3 weeks ago (2017-06-28 14:36:56 UTC) #20
Nico
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 ...
5 months, 3 weeks ago (2017-06-28 14:54:10 UTC) #23
Nico
(and https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_headless_rel/builds/116457 looks like it might be genuinely unhappy about this change)
5 months, 3 weeks ago (2017-06-28 14:54:57 UTC) #24
hans
Thanks for the review! On 2017/06/28 14:54:57, Nico (vacation Jun 30-Jul 11) wrote: > (and ...
5 months, 3 weeks ago (2017-06-28 16:38:25 UTC) #27
hans
Addressed comments and split out sub-changes to https://codereview.chromium.org/2961083002/ and https://codereview.chromium.org/2959203002/ The first of those was ...
5 months, 3 weeks ago (2017-06-28 20:37:35 UTC) #37
Nico
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#newcode135 base/memory/weak_ptr.h:135: // takes up space in the class. ...
5 months, 3 weeks ago (2017-06-28 20:57:03 UTC) #38
hans
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#newcode101 base/memory/weak_ptr.h:101: // consturcted or because they have been invalidated. ...
5 months, 3 weeks ago (2017-06-28 21:16:59 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2963623002/140001
5 months, 3 weeks ago (2017-06-28 21:17:34 UTC) #43
commit-bot: I haz the power
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_asan_rel_ng/builds/402239)
5 months, 3 weeks ago (2017-06-28 23:25:53 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2963623002/160001
5 months, 3 weeks ago (2017-06-28 23:41:12 UTC) #48
commit-bot: I haz the power
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_swarming_rel/builds/210267)
5 months, 3 weeks ago (2017-06-29 01:15:37 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2963623002/160001
5 months, 3 weeks ago (2017-06-29 10:02:59 UTC) #52
commit-bot: I haz the power
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_rel_ng/builds/460568)
5 months, 3 weeks ago (2017-06-29 12:22:53 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2963623002/160001
5 months, 3 weeks ago (2017-06-29 12:29:27 UTC) #56
Nico
On 2017/06/29 12:22:53, commit-bot: I haz the power wrote: > Try jobs failed on following ...
5 months, 3 weeks ago (2017-06-29 12:32:23 UTC) #57
hans
On 2017/06/29 12:32:23, Nico (vacation Jun 30-Jul 11) wrote: > On 2017/06/29 12:22:53, commit-bot: I ...
5 months, 3 weeks ago (2017-06-29 15:04:24 UTC) #58
commit-bot: I haz the power
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_rel_ng/builds/460664)
5 months, 3 weeks ago (2017-06-29 15:12:58 UTC) #60
hans
On 2017/06/29 15:04:24, hans wrote: > On 2017/06/29 12:32:23, Nico (vacation Jun 30-Jul 11) wrote: ...
5 months, 3 weeks ago (2017-06-29 16:23:13 UTC) #61
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2963623002/180001
5 months, 3 weeks ago (2017-06-29 16:24:01 UTC) #64
hans
On 2017/06/29 16:23:13, hans wrote: > Without my patch it overflows at 322 tasks deep. ...
5 months, 3 weeks ago (2017-06-29 16:46:39 UTC) #65
Nico
If it's just that one test collection, and only in debug builds due to sequencechecker, ...
5 months, 3 weeks ago (2017-06-29 16:52:49 UTC) #66
hans
On 2017/06/29 16:52:49, Nico (vacation Jun 30-Jul 11) wrote: > If it's just that one ...
5 months, 3 weeks ago (2017-06-29 16:57:09 UTC) #68
commit-bot: I haz the power
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/526f714c9ae172c3b16581b7e11eb035fd14274e
5 months, 3 weeks ago (2017-06-29 18:22:57 UTC) #71
mgersh
On 2017/06/29 18:22:57, commit-bot: I haz the power wrote: > Committed patchset #10 (id:180001) as ...
5 months, 3 weeks ago (2017-06-29 21:23:51 UTC) #72
maniscalco
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 ...
5 months, 3 weeks ago (2017-06-29 22:02:34 UTC) #73
hans
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/2966633002/ by hans@chromium.org. ...
5 months, 3 weeks ago (2017-06-29 22:02:48 UTC) #74
maniscalco
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/2969513003/ by maniscalco@chromium.org. ...
5 months, 3 weeks ago (2017-06-29 22:03:47 UTC) #75
hans
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 ...
5 months ago (2017-07-17 09:20:37 UTC) #78
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2963623002/240001
5 months ago (2017-07-17 09:22:10 UTC) #82
commit-bot: I haz the power
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/3856eab55bb241fb054a878d9922e65e7b5b0931
5 months ago (2017-07-17 12:03:45 UTC) #85
gab
Just saw this in weak_ptr.h and was curious so I came here :), drive-by lgtm ...
5 months ago (2017-07-18 20:31:08 UTC) #87
hans
5 months ago (2017-07-19 08:08:23 UTC) #88
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 0eb02b776