New issue
Advanced search Search tips

Issue 738167 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

0.2% regression in sizes at 483427:483427

Project Member Reported by tdres...@chromium.org, Jun 29 2017

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=738167

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgnpLNugkM


Bot(s) for this bug's original alert(s):

linux
Owner: h...@chromium.org
Status: Assigned (was: Untriaged)
Looks like a slightly larger size increase than you predicted.

Comment 3 by h...@chromium.org, 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.
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Is this fixed with the revert?

Comment 6 by h...@chromium.org, Jul 17 2017

Status: Fixed (was: Assigned)
I assume so.

Sign in to add a comment