Issue metadata
Sign in to add a comment
|
0.2% regression in sizes at 483427:483427 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jun 29 2017
Looks like a slightly larger size increase than you predicted.
,
Jun 29 2017
Hmm, that's 224 KB :-/ The only difference between my build and the official one is that I disabled cfi. Perhaps the other changes (https://codereview.chromium.org/2959203002 and https://codereview.chromium.org/2961083002) yield larger size wins than I measured locally... Also, if the size reduction on Android is still as expected, perhaps the Linux size is worth it.
,
Jun 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/24046fd36c1a132243a7ffd279f59734f947d223 commit 24046fd36c1a132243a7ffd279f59734f947d223 Author: hans <hans@chromium.org> Date: Thu Jun 29 22:07:42 2017 Revert of Make base::WeakPtr::Get() fast (patchset #10 id:180001 of https://codereview.chromium.org/2963623002/ ) Reason for revert: This seems to have broken Cronet tests and Linux TSan. See bugs. Original issue's description: > 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/+/526f714c9ae172c3b16581b7e11eb035fd14274e TBR=thakis@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 728324 , 738167 , 738183 , 738193 Review-Url: https://codereview.chromium.org/2966633002 Cr-Commit-Position: refs/heads/master@{#483509} [modify] https://crrev.com/24046fd36c1a132243a7ffd279f59734f947d223/base/memory/weak_ptr.cc [modify] https://crrev.com/24046fd36c1a132243a7ffd279f59734f947d223/base/memory/weak_ptr.h [modify] https://crrev.com/24046fd36c1a132243a7ffd279f59734f947d223/base/message_loop/message_loop_test.cc
,
Jul 6 2017
Is this fixed with the revert?
,
Jul 17 2017
I assume so. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by tdres...@chromium.org
, Jun 29 2017