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

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


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

android-nexus7v2

=== 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 : Expand WTF::StringView's API to be more like StringPiece.
Author  : esprehn
Commit description:
  
StringView no longer owns the string passed into it, and can now wrap
a raw ptr to some characters.

This allows us to leverage the inline strlen optimization where the
compiler will embed the length of literal strings into the binary. It
also allows the deletion many overloaded methods that used to take
an LChar*, UChar* or String and can now just take a StringView instead.

For example the two constructors in TextRun are now a single one that
takes a StringView. This needed to be done in this patch to avoid
ambiguous constructors.

Future patches will replace CSSParserString with StringView, and also
vastly simplify the huge number of overloads on various methods. We'll
also expand the API surface of StringView to include the many useful
operations that StringPiece has.

This was originally committed as:
https://crrev.com/330deea56e27bc760fa52101040a51428bb7f582 but was
reverted due an incorrect assert in the
StringView(const UChar*, unsigned length) constructor. The assert
was incorrectly calling lengthOfNullTerminatedString on the UChar
in the assert, but this constructor is used for byte sequences which
are not null terminated.

BUG= 615174 

Review-Url: https://codereview.chromium.org/2007103003
Cr-Commit-Position: refs/heads/master@{#396942}
Commit  : 9b0aad1eca55ac75f437789514224e9b922fe514
Date    : Tue May 31 21:53:32 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev   N  Good?
chromium@396909  155.058  2.28613   5  good
chromium@396925  150.502  1.861     5  good
chromium@396934  153.049  0.528843  5  good
chromium@396938  151.722  0.697432  5  good
chromium@396940  149.858  0.749531  5  good
chromium@396941  150.437  1.7665    5  good
chromium@396942  132.065  1.15437   5  bad    <--

Bisect job ran on: android_nexus7_perf_bisect
Bug ID: 616577

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.parser
Test Metric: textarea-parsing/textarea-parsing
Relative Change: 14.83%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus7_perf_bisect/builds/2988
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9011026949629270352


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

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

I'm pretty sure this is because I removed the StringBuiler::append optimization that reuses the String the first time you call append.

The code used to have:

        // If we're appending to an empty string, and there is not a buffer (reserveCapacity has not been called)
        // then just retain the string.
        if (!m_length && !m_buffer) {
            m_string = string;
            m_length = string.length();
            m_is8Bit = m_string.is8Bit();
            return;
        }

We probably need to add a StringImpl* m_impl member to StringView, and then make StringView::set(StringImpl&, unsigned offset, unsigned length) have a special path for offset == 0 && length == impl.length() that stores the m_impl so StringView::toString()  can reuse the impl in the common case of no specialized offset/length. We probably want that optimization in general so we can use StringView in more places in the code without having more copies.

This benchmark creates a string that's 65536 chars long, the limit right before we start splitting text nodes. I suspect if we made the string 65537 chars long we would have seen this same regression before my patch due to the added string copy.

I don't think you need to be too nervous about the micro-benchmarks. You can mark it as WontFix when you identify the reason and you don't think it's just gaming micro-benchmarks.

However, the optimization you proposed in #3 is nice to have in general.

It also shows up on speedometer: https://bugs.chromium.org/p/chromium/issues/detail?id=617519

No clue why this is not merged into this issue yet.

Lets see if this recovers after https://codereview.chromium.org/2033293002

The speedometer graph is very noisy too, not sure what to make of it. We really need graph smoothing or something :/

Looking at the graph I *think* the Speedometer regression looks recovered? It came back up for the jQuery test on mobile, the others are so noisy, but they do look fixed to me.

The textarea-parsing test didn't recover, I'm not sure what's different on the android-nexus7v2 machine. I can't reproduce the regression on Mac with a profiler, and I don't have a N7v2 to profile. I'm wondering if the regression was actually the v8 roll?

sullivan@ I'm not sure what to make of these graphs. The perf dashboard is super hard to use, it lists so many bots, and most of them have no data for years. I also can't tell what tests are running, or why the ref build massively slower than the real build for some of the tests? Who owns the perf dashboard UX?

I own the perf dashboard UX currently, while aiolos@ is ramping up. We aren't making changes in the short term while we update dashboard + trace-viewer to polymer 1.0 (from 0.5) but we're going to revisit in Q3.

In the short term, I'm working on https://github.com/catapult-project/catapult/issues/1701 to delete all the old timeseries and bots. (Doesn't change the UI, but makes what's there easier to use)

Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

> The textarea-parsing test didn't recover, I'm not sure what's different on the android-nexus7v2 machine. I can't reproduce the regression on Mac with a profiler, and I don't have a N7v2 to profile. I'm wondering if the regression was actually the v8 roll?

That blink_perf.parser/textarea-parsing metric has recovered though it was unrelated to https://codereview.chromium.org/2033293002.

All other metrics have also recovered. So marking this issue as fixed. 

sullivan@: do you need to file a separate bug for issues mentioned in #8 or are they all covered by https://github.com/catapult-project/catapult/issues/1701  

We definitely have tracking on our side for the UX issues here.