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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgxuratwoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgxo-4pAoM


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

chromium-rel-win7-gpu-nvidia
win-zenbook

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

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

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

Hi esprehn@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 : StringBuilder::append should not use reserveCapacity.
Author  : esprehn
Commit description:
  
reserveCapacity allocates the exact capacity specified which means
when we had a 16bit buffer in StringBuilder and were appending 8bit
strings to it we were constantly reallocating and copying the buffer.

Instead we can just call Vector::append() directly since it has a
template overload that can accept implicitly convertible types.

I noticed this when looking at lever.co's web app loading in
Instruments, it was spending 15% of the main thread time inside
TextResource::decodedText which was 83% memmove, and 11% munmap.

Switching to Vector::append should restore the correct size
doubling behavior when appending LChars to a UChar StringBuilder
and make this much faster.

This was a regression from when I switched to using a Vector
inside StringBuilder in:
https://codereview.chromium.org/2046353002

Review-Url: https://codereview.chromium.org/2192293002
Cr-Commit-Position: refs/heads/master@{#408869}
Commit  : 25fdbad21c66328e5ada9b9b436d3a8c146ca728
Date    : Sat Jul 30 07:38:36 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@408865  7007006  17854.1  5  good
chromium@408867  7007006  14481.5  5  good
chromium@408868  7017656  13768.9  5  good
chromium@408869  7138078  55106.0  5  bad    <--

Bisect job ran on: winx64nvidia_perf_bisect
Bug ID: 633197

Test Command: src/tools/perf/run_benchmark -v --browser=release_x64 --output-format=chartjson --upload-results --also-run-disabled-tests system_health.memory_desktop
Test Metric: load_social-memory:chrome:all_processes:reported_by_chrome:partition_alloc:effective_size_avg/load_social_pinterest
Relative Change: 1.87%
Score: 99.0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/winx64nvidia_perf_bisect/builds/1804
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9004248348264025232


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

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

How do I run this benchmark locally?

It's a little buried in the bisect output, we're working on cleaning it up. For future reference, you can see it under "Test Command":

tools/perf/run_benchmark -v --browser=release_x64 --output-format=chartjson --upload-results --also-run-disabled-tests system_health.memory_desktop

Also note that this only seems to repro on x64 windows. We have perf trybots which have the same config as the testers if you don't have that setup locally: https://www.chromium.org/developers/telemetry/performance-try-bots

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e5dd14ac89435d384867f678106374f471dd414a

commit e5dd14ac89435d384867f678106374f471dd414a
Author: esprehn <esprehn@chromium.org>
Date: Wed Sep 14 00:25:57 2016

Release the buffer in StringBuilder::toString() and toAtomicString().

Various places in the code like FetchDataLoader and WorkerScriptLoader
keep a StringBuilder as a data member to buffer data and then call
toString() on it later. By clearing the buffer in the string conversion
methods we avoid having two copies of the values in memory persistently.

This restores the behavior from before I switched it to a Vector in
https://codereview.chromium.org/2046353002 where toString() and
toAtomicString() would always clear the buffer when converting to a
string because they called reifyString.

While I haven't been able to confirm this is the cause, this seems a
possible candidate for the small regression on system health benchmarks.
It's also just generally better behavior since it avoids an accidental
2x memory increase from a StringBuilder member.

This also makes appending individual characters cheaper by removing the
branch from resetting m_string every time.

BUG= 633197 

Review-Url: https://codereview.chromium.org/2335193005
Cr-Commit-Position: refs/heads/master@{#418437}

[modify] https://crrev.com/e5dd14ac89435d384867f678106374f471dd414a/third_party/WebKit/Source/wtf/text/StringBuilder.cpp
[modify] https://crrev.com/e5dd14ac89435d384867f678106374f471dd414a/third_party/WebKit/Source/wtf/text/StringBuilder.h

Not sure what to make of this benchmark now, there's several other regressions since this, the latest one is a 55% regression 6 days ago (the 8th). I'm not sure how to even tell from the graph if this patch fixed it because the test has regressed so much.

Is someone else looking at the 50% regression?

I don't see an improvement from my patch on this benchmark, but I also see that after this bug was filed we improved a few times to before the baseline where this bug was filed, so I think this is WontFix. My patch does in theory increase memory temporarily in places where we would reserve space before, ex.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Element.cpp?q=Element::textFromChildren&sq=package:chromium&dr=CSs&l=2835

previously since the buffer inside StringBuilder was itself a StringImpl, we were copying the bytes directly into the result string. Now that we use a Vector we're allocating 2x the memory very temporarily inside ::toString().

Someone should really look into the giant 50% regression on this benchmark though, I don't see a bug filed on the graph. :/