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

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


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

chromium-rel-win7-dual

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

=== 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 : Out-of-line WTF::String's destructor.
Author  : dcheng
Commit description:
  
This saves ~504KB in the official Linux x64 binary.

BUG=none

Review-Url: https://codereview.chromium.org/2516123002
Cr-Commit-Position: refs/heads/master@{#433737}
Commit  : 2ce3d703c50150ebe5cd6c3f1e2bf4a19f590766
Date    : Tue Nov 22 02:08:03 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N   Good?
chromium@433731  103.93   5.56463  25  good
chromium@433734  104.278  2.6077   25  good
chromium@433736  105.571  3.86769  25  good
chromium@433737  114.169  3.0005   25  bad    <--
chromium@433742  114.399  4.89539  25  bad
chromium@433752  114.063  4.63715  25  bad
chromium@433773  113.049  4.99883  25  bad

Bisect job ran on: win_perf_bisect
Bug ID: 668166

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.dom
Test Metric: modify-element-title/modify-element-title
Relative Change: 8.77%

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_perf_bisect/builds/7051
Job details: https://chromeperf.appspot.com/buildbucket_job_status/8995190446732620192


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

| 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!

Well, so much for that.

https://chromeperf.appspot.com/group_report?rev=433737

While test itself seems a bit contrived (it just flips the title back and forth in a tight loop), 10.7% is a lot to regress.

I'm honestly not sure what conclusion to draw.

One question is how much we trust our perf infrastructure to catch issues in non-micro-benchmarks and whether we're OK with regressing micro-benchmarks if they don't regress real-world content. In principle, we should be IMO.

All the tests that regress are doing a very cheap and relatively rare operation half a million times. So, it's not clear that this would impact real content. getElementById and setting classname/attributes slowed down (10-20%), but the tests are literally measuring the bindings overhead, not the enormous amount of style recalc and DOM work that results from changing those. So, I suspect those won't be real-world regressions.

At one point, bindings team was working on making accessors like this inlined. I don't know if that's still being worked on or how it might change the effect of this patch. Could make it better or worse. :)

I think this data is showing that any JS binding that takes a string argument is now slower? Is it really just the cost of the destructor for the one string argument?

Maybe it's worth discussing this on platform-architecture-dev. I'm really not sure how to make a good decision here.

Yeah, I think it would be good to figure out how to balance the tradeoffs here. For the moment though, I've opted to revert to be safe.

Oh, totally. I didn't mean to imply you should leave it in the tree.

This change was reverted, but we got nearly the whole memory gain of the original change (without the perf hit) here:
https://chromium.googlesource.com/chromium/src/+/1b802780176a02684692e9dea6bb70a37e3d767c

(The CL responsible was already reverted, so just closing this now)