All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=645563

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


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

android-nexus5
android-nexus6

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/9001971538526127152

=== Auto-CCing suspected CL author dcheng@chromium.org ===

Hi dcheng@chromium.org, the bisect results pointed to your CL below as possibly
causing a regression. Please have a look at this info and see whether
your CL be related.


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Implement WTF::WeakPtr in terms of base::WeakPtr
Author  : dcheng
Commit description:
  
BUG=none

Review-Url: https://codereview.chromium.org/2300813003
Cr-Commit-Position: refs/heads/master@{#416133}
Commit  : 288b352ea69e3cff2540ac701bb181485d423192
Date    : Fri Sep 02 00:02:00 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@416118  37455.3  140.909  6  good
chromium@416126  37555.2  62.5716  5  good
chromium@416130  37313.6  142.677  5  good
chromium@416132  37343.2  292.744  5  good
chromium@416133  37907.2  226.034  5  bad    <--
chromium@416148  37848.0  226.335  8  bad

Bisect job ran on: android_nexus5_perf_bisect
Bug ID: 645563

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests webrtc.peerconnection
Test Metric: vm_private_dirty_final_renderer/vm_private_dirty_final_renderer
Relative Change: 1.00%
Score: 99.5

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus5_perf_bisect/builds/4110
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9001971538526127152


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5864438895214592

| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Tests>AutoBisect.  Thank you!

OK, I think this is because base's WeakPtr stores the pointer to deref in the WeakPtr itself instead of the shared state. WTF's stored it in the shared state. Are there perf trybots somewhere that I can use to see what the impact of such a change would be? I know there's instructions somewhere... but I can never find them. It would be kind of nice if they were in the bug =)

Yep, you can get there from the "more information" link, it eventually leads here:
http://www.chromium.org/developers/telemetry/performance-try-bots

OK, so I relanded the patch that caused this original regression (it was reverted for an unrelated reason). Should I expect another bug for fixing this? I looked at the graphs, and it looks like that revision (https://codereview.chromium.org/2329243002/) hasn't hit the bots yet.

As for the fix, I tried writing a patch, but the problem is SupportsWeakPtr: it allows a Base and a Derived to both vend WeakPtrs from a single SupportsWeakPtr<Base>. This is a problem, because it means I can't use the trick from WTF to stash a pointer in the shared internal state: the pointer may need to be offset, but we don't have enough type information to do that correctly.

danakj@, me, and several others are in agreement that SupportsWeakPtr should probably be removed, but that's a non-trivial project: there are several hundred usages throughout Chrome.

We could split base::WeakPtr into something lower level that WTF could use, and the slower base::WeakPtr stuff can be built on top? That gets the code reuse without the perf hit. :)

I'd rather we kept WTF::WeakPtr if using base is a perf hit.

Re #6:  Yes, we should remove SupportsWeakPtr! But, since that's a crazy amount of work... Do you have an example of the Base/Derived case you mention?

#7: My understanding is this is a memory regression, not a perf regression. In fact, the WTF version has an indirection due to storing the pointer in the internal state rather than in the WeakPtr itself.

#8: What sort of example are you looking for?

I don't think there's a clean way to split base::WeakPtr's internals out, and there's a lot of downside to having two WeakPtrs (such as their threading restrictions not quite matching =). I'll see if I can dedicate a background thread to getting rid of SupportsWeakPtr, but it's going to be a long road.

Sorry, I mean memory regression, not *speed* regression. Obviously it's still a perf regression.

What is the object that uses WeakPtr in blink that's causing the memory regression? What does the benchmark do that uses more memory now? How many objects of that type are  being created? Do we expect developers to actually create that many objects? :)

I have no idea what could be using WeakPtr that much: the benchmarks show a several hundred KB change, which is huge. There's also not very many WeakPtrFactory objects in blink, and none that look like they'd only be involved in webrtc stuff.

Also, thinking about this more, I have a hard time believing it's just due to this change. It's an extra field in WeakPtr, which is 8 bytes. You can fit a lot of pointers into several hundred KB. The perf graph also doesn't show a noticeable improvement when the original patch was reverted in r416838.

Perf sheriff ping

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8998327622867020240

=== Auto-CCing suspected CL author dcheng@chromium.org ===

Hi dcheng@chromium.org, the bisect results pointed to your CL below as possibly
causing a regression. Please have a look at this info and see whether
your CL be related.


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Implement WTF::WeakPtr in terms of base::WeakPtr
Author  : dcheng
Commit description:
  
BUG=none

Review-Url: https://codereview.chromium.org/2300813003
Cr-Commit-Position: refs/heads/master@{#416133}
Commit  : 288b352ea69e3cff2540ac701bb181485d423192
Date    : Fri Sep 02 00:02:00 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@416115  40184.5  130.746  8  good
chromium@416132  40197.5  153.852  8  good
chromium@416133  40624.0  145.63   5  bad    <--
chromium@416134  40584.8  98.4033  5  bad
chromium@416136  40777.6  119.619  5  bad
chromium@416140  40660.0  151.974  5  bad
chromium@416148  40594.4  166.099  5  bad

Bisect job ran on: android_nexus6_perf_bisect
Bug ID: 645563

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests webrtc.peerconnection
Test Metric: vm_private_dirty_final_renderer/vm_private_dirty_final_renderer
Relative Change: 1.06%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus6_perf_bisect/builds/2677
Job details: https://chromeperf.appspot.com/buildbucket_job_status/8998327622867020240


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5880574693081088

| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Tests>AutoBisect.  Thank you!

TimerBase uses weakptrs, maybe that's the source?

We don't monitor this metric anymore, and there's been no movement on the bug in 10 months. WontFix-ing.