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

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=3307a9ee090d931ec97aa6b1b1258a4a9e3f7a8e39341fc48b8dcfec04c7f198


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

health-plan-clankium-low-end-phone
health-plan-clankium-phone
perf-go-phone-1024
perf-go-phone-512

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

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

Hi bstell@chromium.org, the bisect results pointed to your CL, please take a look at the
results.


=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : Brian Stell
  Commit : e3a5b1e4020e2acb40ddd1f47770e9db9153ad9c
  Date   : Tue Jul 17 16:47:34 2018
  Subject: Add IsStructurallyValidLanguageTag() routine.

Bisect Details
  Configuration: clankium-low-end-phone-perf-bisect
  Benchmark    : memory.top_10_mobile
  Metric       : memory:chrome:all_processes:reported_by_os:system_memory:native_heap:proportional_resident_size_avg/background/after_http_m_intl_taobao_com_group_purchase_html
  Change       : 3.88% | 12501553.3333 -> 12986246.6667

Revision                                                     Result                   N
android-chrome@e5eafcf94f                                    12501553 +- 231058       6      good
android-chrome@74662f91c8                                    12518279 +- 197767       6      good
android-chrome@74662f91c8,chromium@575926                    12605660 +- 215171       6      good
android-chrome@74662f91c8,chromium@575929                    12626823 +- 190037       6      good
android-chrome@74662f91c8,chromium@575929,v8@46a78fbedf      13033521 +- 48044.8      6      good
android-chrome@74662f91c8,chromium@575929,v8@e3a5b1e402      13480839 +- 130325       6      bad       <--
android-chrome@74662f91c8,chromium@575929,v8@16af1baac4      12978168 +- 120038       9      bad
android-chrome@74662f91c8,chromium@575930                    13035512 +- 226784       9      bad
android-chrome@74662f91c8,chromium@575931                    13227740 +- 196629       6      bad
android-chrome@74662f91c8,chromium@575935                    13046663 +- 151617       6      bad
android-chrome@1032e70048                                    12986247 +- 171341       6      bad

Please refer to the following doc on diagnosing memory regressions:
  https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-chrome --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=http.m.intl.taobao.com.group.purchase.html memory.top_10_mobile

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8940201563618267040


For feedback, file a bug with component Speed>Bisection

This seems really odd to me.

Currently IsStructurallyValidLanguageTag is referenced by Intl::CanonicalizeLanguageTag but that is not being called. I have a cl ready to connect Intl::CanonicalizeLanguageTag, https://chromium-review.googlesource.com/c/v8/v8/+/1144524 , but it has not been checked in.

+perezju, any tips on where to look for why this CL could have caused a 400kb memory increase?

This is definitely a bit odd. I did expect https://chromium-review.googlesource.com/c/v8/v8/+/1117303 to cause a regression in memory as it increases the size of the isolate, but it's not clear to me why https://chromium-review.googlesource.com/c/v8/v8/+/1119103 is marked as the culprit here.

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

Looking at the bisect, it's a little unclear if it got the right CL:


android-chrome@74662f91c8,chromium@575929                    12626823 +- 190037       6      good
android-chrome@74662f91c8,chromium@575929,v8@46a78fbedf      13033521 +- 48044.8      6      good
android-chrome@74662f91c8,chromium@575929,v8@e3a5b1e402      13480839 +- 130325       6      bad       <--
android-chrome@74662f91c8,chromium@575929,v8@16af1baac4      12978168 +- 120038       9      bad


Re-kicking

https://chromium-review.googlesource.com/c/v8/v8/+/1117303 adds:

1. //third_party/icu as a dep to
* torque_generated_initializers
* v8_init
* fuzzer_support
* wasm_module_runner
* lib_wasm_fuzzer_common

2. isolate.h
* 3 pointers 
* 3 getters and 1 setter

3. isolate.cc:
* code to init the 3 pointers to null
* code to clear the 3 pointers

4. intl-objects.cc
* code to build the regexps (never called)
* 3 functions to get the regexps (never called)

The regex components take about 500-1000 characters.

How can any of this add 400K?

The specific bisect seems to indicate https://chromium-review.googlesource.com/c/v8/v8/+/1119103

This is an even earlier cl that only changes intl-object.cc:

1. replaces 1 if statement with 3 CHECK statements

2. adds 2 functions:
* AsciiToLower (only called by IsStructurallyValidLanguageTag)
* IsStructurallyValidLanguageTag (never called)

This issue is about an increase in memory usage of about 400K.

v8:7978 is about:
1. building the regexps from fewer/larger char* rather many small std::string
2. using non-capturing regexp groups.

The number of std::string elements is a few dozen involving a few hundred total chars.

The non-capturing groups would add slightly to the number of chars. If the code is not being called then capture/non-capture would not have any affect on the issue indicated in this bug.

=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : Brian Stell
  Commit : e3a5b1e4020e2acb40ddd1f47770e9db9153ad9c
  Date   : Tue Jul 17 16:47:34 2018
  Subject: Add IsStructurallyValidLanguageTag() routine.

Bisect Details
  Configuration: clankium-low-end-phone-perf-bisect
  Benchmark    : memory.top_10_mobile
  Metric       : memory:chrome:all_processes:reported_by_os:system_memory:native_heap:proportional_resident_size_avg/background/after_http_m_intl_taobao_com_group_purchase_html
  Change       : 3.58% | 12606610.8571 -> 12993390.2857

Revision                                                     Result                   N
android-chrome@e5eafcf94f                                    12606611 +- 152992       14      good
android-chrome@74662f91c8                                    12715009 +- 237394       14      good
android-chrome@74662f91c8,chromium@575926                    12680753 +- 189712       9       good
android-chrome@74662f91c8,chromium@575929                    12652081 +- 184862       6       good
android-chrome@74662f91c8,chromium@575929,v8@46a78fbedf      12933340 +- 238931       6       good
android-chrome@74662f91c8,chromium@575929,v8@e3a5b1e402      13341916 +- 96525.8      6       bad       <--
android-chrome@74662f91c8,chromium@575929,v8@16af1baac4      13048824 +- 224880       9       bad
android-chrome@74662f91c8,chromium@575930                    13092743 +- 163441       6       bad
android-chrome@74662f91c8,chromium@575931                    13020835 +- 200752       9       bad
android-chrome@74662f91c8,chromium@575935                    13067256 +- 171772       9       bad
android-chrome@1032e70048                                    12993390 +- 222134       14      bad

Please refer to the following doc on diagnosing memory regressions:
  https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-chrome --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=http.m.intl.taobao.com.group.purchase.html memory.top_10_mobile

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8940099675238003136


For feedback, file a bug with component Speed>Bisection

The metric is noisy, so this is proving hard to bisect. However the bisect in #13 is a lot clearer and definitive.

I would suggest looking into a couple of traces, before:
https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/after_http_m_intl_taobao_com_group_purchase_html_2018-07-18_01-08-43_1710.html

after:
https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/after_http_m_intl_taobao_com_group_purchase_html_2018-07-18_04-20-52_16832.html

If you look into the renderer process -> private footprint -> native heap, it grew from 153,541.9 KiB to 154,349.9 KiB (i.e. +808 KiB).

That might help pinpointing where the extra memory is coming from.

Please let me know if I'm understanding this correctly.

Commit e3a5b1e4020e2acb40ddd1f47770e9db9153ad9c points to cl https://chromium-review.googlesource.com/c/v8/v8/+/1119103

Assuming I have the cl correct let's look at it.

This cl makes 3 changes.

#1 This changes BuildLanguageTagRegexps.
* This is called by 3 functions:
** GetLanguageSingletonRegexMatcher
** GetLanguageTagRegexMatcher
** GetLanguageVariantRegexMatcher
All 3 of these functions are only called by IsStructurallyValidLanguageTag.

#2 This cl adds AsciiToLower.
This is only called by IsStructurallyValidLanguageTag.

#3 This cl adds IsStructurallyValidLanguageTag
This is only called by Intl::CanonicalizeLanguageTag

At the time of this cl I had not hooked up Intl::CanonicalizeLanguageTag.

Looking at the source even today Intl::CanonicalizeLanguageTag is never called.

Assuming my previous comment is correct:

Please help me understand how this cl might be related to the memory change.
* Is the code called?
* If the code is not being called can it somehow increase native heap?

The CL identified is correct.

+ssid, see #14-#16, any clues on what could be going in here in terms of native heap increasing?

Let me know if I'm missing/misunderstanding something.

Here is a cl that prepends a "XXX_" to IsStructurallyValidLanguageTag Intl::CanonicalizeLanguageTag to show they are not being called:

https://chromium-review.googlesource.com/c/v8/v8/+/1151405

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/16b425f7a40000

^^ I've just created that job to test if your patch in #18 causes any changes.

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/16b425f7a40000

Can I get some guidance on how to view or understand these results?

I'm not sure what the job in #19 is trying to prove? The CL in #18, by virtue of still compiling, proves that the code added in the blamed suspect #13 indeed does not have any references (for now; we already knew this). Renaming dead code is not expected to have any performance impact.

e3a5b1e4020e2acb40ddd1f47770e9db9153ad9c does not change memory allocations. Any bisect job claiming otherwise cannot be trusted. (The commit shouldn't even change binary size, because the linker should drop unused code, but I haven't verified this.)

The V8 commit right before, 46a78fbedfcd458f5d35097bdf1a9947644f4b0e, on the other hand, shuffles memory around: from the binary to the heap. A couple hundred kilobytes seems plausible to me. +jgruber to confirm. Note the corresponding decrease in binary size of about 700-800KB around Chromium commit 575930: https://chromeperf.appspot.com/report?sid=30e1c210de87ce32396a3f784119944597968aac9da7d0468ac4c265a43ca989

Since that change is hopefully temporary, I think we can close this bug. Please reopen if you disagree.

bstell, sorry for the noise. Perf results, especially overall memory consumption, are notoriously noisy; but they're the best we have for keeping an eye on this crucial metric.

perezju/sullivan: is it worth investigating why both bisect jobs were so useless here? Maybe larger N could mitigate the flakiness?

+simonhatch, is it possible bisect could be off-by-one here?

perezju: am I reading the tryjob results correctly, that with the CL we're actually seeing 400kib LESS memory usage? see screenshot.

Deleted: results2.PNG 74.2 KB

	results2.PNG 74.2 KB View Download

Thanks everyone for helping bring this to a resolution.

yeah there's something a little funny about the last run, doesn't really match any of the others on either the good or bad set.

#23 Jakob, we indeed expect memory reshuffling from 46a78fbedfcd458f5d35097bdf1a9947644f4b0e from the binary into the JS heap. But 8 out of 10 of the associated alerts are for 

 memory:chrome:all_processes:reported_by_os:system_memory:native_heap

AFAIK the JS heap isn't included in that, is it?

Although your theory does seem supported by the fact that graph signals coincide with landing/reverting embedded builtins:

Landed:

commit f5a8352b0fbba0d88104de7949a0d0c238765cf8
Author: Sigurd Schneider <sigurds@chromium.org>
Date:   Tue Jun 19 12:52:53 2018 +0200

    [embedded-builtins] Enable on all arches except x86 for benchmarks

Graph [0] (system_memory:native_heap) dips at point id: 1529643774.

Reverted:

commit 46a78fbedfcd458f5d35097bdf1a9947644f4b0e
Author: Sigurd Schneider <sigurds@chromium.org>
Date:   Tue Jul 17 08:06:01 2018 +0000

    Revert "[embedded-builtins] Enable on all arches except x86 for benchmarks"

Graph rises at point id: 1531917711. A delta of ~450K is also in the ballpark. We'll know for sure when yesterday's reland hits the graphs.

[0] https://chromeperf.appspot.com/report?sid=6dc6557138dc505ae565c50f3ccdd8174be6fb718021631ed32a60e9641886f9&start_rev=1528246901&end_rev=1532660919

Re #24 your reading is correct, but the change is not regarded a statistically significant (which is consistent with the "just renaming dead code" assumption).

Agree it's strange that a CL like v8@e3a5b1e402 would be detected twice as the culprit by two different bisect jobs.

Anyway, because this is on the legacy bisect system, and the magnitude of the regression is not huge, also agree it's best not to spend more time on this and leave WontFix'ed.

Graphs are back to baseline with the reland at Point ID: 1532691316: https://chromeperf.appspot.com/report?sid=6dc6557138dc505ae565c50f3ccdd8174be6fb718021631ed32a60e9641886f9