New issue
Advanced search Search tips

Issue 837498 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android , Windows
Pri: 3
Type: Bug



Sign in to add a comment

More Memory_Experimental UKM reports after page loaded.

Project Member Reported by tasak@google.com, Apr 27 2018

Issue description

Currently Memory_Experimental UKM report is recorded every 30 minutes.
However, renderers' memory usage seems to be large (several minutes?) after page loaded.
To understand real web, I would like to report Memory_Experimental UKM 1, 5, 10, and 15 minutes after page loaded.
  
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 11 2018

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

commit 02704a2233f13ef61d3705a77b978633e9697056
Author: Takashi SAKAMOTO <tasak@google.com>
Date: Fri May 11 11:37:54 2018

Make ProcessMemoryMetricsEmitter to emit memory metrics of the specified process

This is [1/2] of the patch: https://chromium-review.googlesource.com/c/chromium/src/+/925987 to add Memory_Experimental UKM 1, 5, 10 and 15 minutes after page loaded.
ProcessMemoryMetricsEmitter is used to record Memory_Experimental UKM and emits the all processes' memory metrics.
Since we would like to use the same code to record the UKM, we modify ProcessMemoryMetricsEmitter to record the specified page's Memory_Experimental UKM, i.e. the specified renderer's Memory_Experimental UKM.

Change-Id: I99242123529a53c97e91153545a40e0f7aa21f26

Bug:  837498 
Change-Id: I99242123529a53c97e91153545a40e0f7aa21f26
Reviewed-on: https://chromium-review.googlesource.com/1032150
Commit-Queue: Takashi Sakamoto <tasak@google.com>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557843}
[modify] https://crrev.com/02704a2233f13ef61d3705a77b978633e9697056/chrome/browser/metrics/process_memory_metrics_emitter.cc
[modify] https://crrev.com/02704a2233f13ef61d3705a77b978633e9697056/chrome/browser/metrics/process_memory_metrics_emitter.h
[modify] https://crrev.com/02704a2233f13ef61d3705a77b978633e9697056/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc
[modify] https://crrev.com/02704a2233f13ef61d3705a77b978633e9697056/services/resource_coordinator/memory_instrumentation/coordinator_impl.h
[modify] https://crrev.com/02704a2233f13ef61d3705a77b978633e9697056/services/resource_coordinator/memory_instrumentation/coordinator_impl_unittest.cc
[modify] https://crrev.com/02704a2233f13ef61d3705a77b978633e9697056/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.cc
[modify] https://crrev.com/02704a2233f13ef61d3705a77b978633e9697056/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h
[modify] https://crrev.com/02704a2233f13ef61d3705a77b978633e9697056/services/resource_coordinator/public/cpp/memory_instrumentation/tracing_integration_unittest.cc
[modify] https://crrev.com/02704a2233f13ef61d3705a77b978633e9697056/services/resource_coordinator/public/mojom/memory_instrumentation/memory_instrumentation.mojom

Project Member

Comment 2 by bugdroid1@chromium.org, May 23 2018

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

commit d911f7ffcc03fe645575e959c848a060bcacc0fe
Author: Takashi SAKAMOTO <tasak@google.com>
Date: Wed May 23 04:53:51 2018

[2/2] Report Memory_Experimental UKM 1, 5, 10 and 15 minutes after each tab's page load

We would like to see more Memory_Experimental UKM after page load.
We are thinking that renderer memory usage will quickly grow after page load.
However, currently Memory_Experimental UKM is reported every 30 minutes, and
is not related to any page load.

ProcessMemoryMetricsEmitter's change was https://chromium-review.googlesource.com/1032150
and submitted.

Bug:  837498 
Change-Id: I16eb5997879fb9a16b647969dc9e73503211065c
Reviewed-on: https://chromium-review.googlesource.com/925987
Commit-Queue: Takashi Sakamoto <tasak@google.com>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560948}
[modify] https://crrev.com/d911f7ffcc03fe645575e959c848a060bcacc0fe/chrome/browser/BUILD.gn
[modify] https://crrev.com/d911f7ffcc03fe645575e959c848a060bcacc0fe/chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
[add] https://crrev.com/d911f7ffcc03fe645575e959c848a060bcacc0fe/chrome/browser/resource_coordinator/tab_memory_metrics_reporter.cc
[add] https://crrev.com/d911f7ffcc03fe645575e959c848a060bcacc0fe/chrome/browser/resource_coordinator/tab_memory_metrics_reporter.h
[add] https://crrev.com/d911f7ffcc03fe645575e959c848a060bcacc0fe/chrome/browser/resource_coordinator/tab_memory_metrics_reporter_unittest.cc
[modify] https://crrev.com/d911f7ffcc03fe645575e959c848a060bcacc0fe/chrome/test/BUILD.gn

Project Member

Comment 3 by bugdroid1@chromium.org, May 24 2018

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

commit 60efe19e288d308654ec1ec8f8d4d6d03e4f262a
Author: Gabriel Charette <gab@chromium.org>
Date: Thu May 24 20:31:19 2018

Revert "[2/2] Report Memory_Experimental UKM 1, 5, 10 and 15 minutes after each tab's page load"

This reverts commit d911f7ffcc03fe645575e959c848a060bcacc0fe.

Reason for revert: crbug.com/846426

Original change's description:
> [2/2] Report Memory_Experimental UKM 1, 5, 10 and 15 minutes after each tab's page load
> 
> We would like to see more Memory_Experimental UKM after page load.
> We are thinking that renderer memory usage will quickly grow after page load.
> However, currently Memory_Experimental UKM is reported every 30 minutes, and
> is not related to any page load.
> 
> ProcessMemoryMetricsEmitter's change was https://chromium-review.googlesource.com/1032150
> and submitted.
> 
> Bug:  837498 
> Change-Id: I16eb5997879fb9a16b647969dc9e73503211065c
> Reviewed-on: https://chromium-review.googlesource.com/925987
> Commit-Queue: Takashi Sakamoto <tasak@google.com>
> Reviewed-by: Chris Hamilton <chrisha@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#560948}

TBR=chrisha@chromium.org,tasak@google.com,erikchen@chromium.org,fmeawad@chromium.org,haraken@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  837498 , 846426
Change-Id: I45a2e3df525617e797680f4ddf14582b6f35df0e
Reviewed-on: https://chromium-review.googlesource.com/1072328
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561611}
[modify] https://crrev.com/60efe19e288d308654ec1ec8f8d4d6d03e4f262a/chrome/browser/BUILD.gn
[modify] https://crrev.com/60efe19e288d308654ec1ec8f8d4d6d03e4f262a/chrome/browser/resource_coordinator/tab_helper.cc
[delete] https://crrev.com/6a7c83a9f55a3fd8385d41782c04fe13a47bb066/chrome/browser/resource_coordinator/tab_memory_metrics_reporter.cc
[delete] https://crrev.com/6a7c83a9f55a3fd8385d41782c04fe13a47bb066/chrome/browser/resource_coordinator/tab_memory_metrics_reporter.h
[delete] https://crrev.com/6a7c83a9f55a3fd8385d41782c04fe13a47bb066/chrome/browser/resource_coordinator/tab_memory_metrics_reporter_unittest.cc
[modify] https://crrev.com/60efe19e288d308654ec1ec8f8d4d6d03e4f262a/chrome/test/BUILD.gn

Project Member

Comment 4 by bugdroid1@chromium.org, May 25 2018

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

commit 006522629080281754763876f822bfb63550fa01
Author: Takashi SAKAMOTO <tasak@google.com>
Date: Fri May 25 06:57:15 2018

Reland [2/2] Report Memory_Experimental UKM 1, 5, 10 and 15 minutes after each tab's page load

Fixed telemetry.internal.backends.chrome.tab_list_backend_unittest.TabListBackendTest.testTabIdStableAfterTabCrash.
If discarding a tab, TabMemoryMetricsReporter should not invoke EmitProcessMemoryMetrics for the tab because its process value is not valid.

Bug:  837498 , 846426
Change-Id: Ic23f779b5d64eec90467494a2158c0d6e81ae4b2
Reviewed-on: https://chromium-review.googlesource.com/1073109
Commit-Queue: Takashi Sakamoto <tasak@google.com>
Commit-Queue: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561794}
[modify] https://crrev.com/006522629080281754763876f822bfb63550fa01/chrome/browser/BUILD.gn
[modify] https://crrev.com/006522629080281754763876f822bfb63550fa01/chrome/browser/resource_coordinator/tab_helper.cc
[add] https://crrev.com/006522629080281754763876f822bfb63550fa01/chrome/browser/resource_coordinator/tab_memory_metrics_reporter.cc
[add] https://crrev.com/006522629080281754763876f822bfb63550fa01/chrome/browser/resource_coordinator/tab_memory_metrics_reporter.h
[add] https://crrev.com/006522629080281754763876f822bfb63550fa01/chrome/browser/resource_coordinator/tab_memory_metrics_reporter_unittest.cc
[modify] https://crrev.com/006522629080281754763876f822bfb63550fa01/chrome/test/BUILD.gn

Comment 5 by tasak@google.com, May 29 2018

Cc: haraken@chromium.org
Labels: Merge-Request-68
I would like to merge this change (#561794) into M68.
Because we need this metric for our user study (in the near future). I landed the patch at #557843, but unfortunately the patch was reverted because of failing 1 unit test (#561611).

Project Member

Comment 6 by sheriffbot@chromium.org, May 29 2018

Labels: -Merge-Request-68 Hotlist-Merge-Review Merge-Review-68
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Please add appropriate impacted OS. 

Comment 8 by tasak@google.com, Jun 1 2018

Labels: OS-Chrome OS-Linux OS-Mac OS-Windows
Sure. I would like to measure memory usage of mainly Android (mobile) and Windows (desktop).
However, this patch depends on a component, i.e. TabManager, which is not currently supported on Android. So impacted OS is desktops.

Comment 9 by tasak@google.com, Jun 1 2018

Labels: -OS-Linux -OS-Chrome -OS-Mac OS-Android
I'm sorry. I made the patch work without TabManager.
I marked this bug as Android and Windows.

Labels: -Merge-Review-68 Merge-Approved-68
Approving for M68. Branch:3440

Comment 11 by tasak@google.com, Jun 5 2018

Thank you.

Project Member

Comment 12 by bugdroid1@chromium.org, Jun 5 2018

Labels: -merge-approved-68 merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/74324f8287efdc73b1cc77eecd2f4337403cf36d

commit 74324f8287efdc73b1cc77eecd2f4337403cf36d
Author: Takashi SAKAMOTO <tasak@google.com>
Date: Tue Jun 05 04:00:21 2018

Reland [2/2] Report Memory_Experimental UKM 1, 5, 10 and 15 minutes after each tab's page load

Fixed telemetry.internal.backends.chrome.tab_list_backend_unittest.TabListBackendTest.testTabIdStableAfterTabCrash.
If discarding a tab, TabMemoryMetricsReporter should not invoke EmitProcessMemoryMetrics for the tab because its process value is not valid.

TBR=tasak@google.com

(cherry picked from commit 006522629080281754763876f822bfb63550fa01)

Bug:  837498 , 846426
Change-Id: Ic23f779b5d64eec90467494a2158c0d6e81ae4b2
Reviewed-on: https://chromium-review.googlesource.com/1073109
Commit-Queue: Takashi Sakamoto <tasak@google.com>
Commit-Queue: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#561794}
Reviewed-on: https://chromium-review.googlesource.com/1086669
Reviewed-by: Takashi Sakamoto <tasak@google.com>
Cr-Commit-Position: refs/branch-heads/3440@{#181}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/74324f8287efdc73b1cc77eecd2f4337403cf36d/chrome/browser/BUILD.gn
[modify] https://crrev.com/74324f8287efdc73b1cc77eecd2f4337403cf36d/chrome/browser/resource_coordinator/tab_helper.cc
[add] https://crrev.com/74324f8287efdc73b1cc77eecd2f4337403cf36d/chrome/browser/resource_coordinator/tab_memory_metrics_reporter.cc
[add] https://crrev.com/74324f8287efdc73b1cc77eecd2f4337403cf36d/chrome/browser/resource_coordinator/tab_memory_metrics_reporter.h
[add] https://crrev.com/74324f8287efdc73b1cc77eecd2f4337403cf36d/chrome/browser/resource_coordinator/tab_memory_metrics_reporter_unittest.cc
[modify] https://crrev.com/74324f8287efdc73b1cc77eecd2f4337403cf36d/chrome/test/BUILD.gn

Comment 13 by tasak@google.com, Jun 5 2018

Status: Fixed (was: Started)
I fixed chrome/browser/BUILD.gn conflicts (and locally confirmed the fix was valid) and landed.

Project Member

Comment 14 by bugdroid1@chromium.org, Jun 6 2018

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

commit 5cc974fd9d3bee6cc3bee9ff6c296c4a9c00457a
Author: Michael Moss <mmoss@chromium.org>
Date: Wed Jun 06 00:32:05 2018

Revert "Reland [2/2] Report Memory_Experimental UKM 1, 5, 10 and 15 minutes after each tab's page load"

This reverts commit 74324f8287efdc73b1cc77eecd2f4337403cf36d.

Reason for revert: Breaking official builds with:
  The target //chrome/browser:browser
  generates two object files with the same name:
    obj/chrome/browser/browser/time.o

Original change's description:
> Reland [2/2] Report Memory_Experimental UKM 1, 5, 10 and 15 minutes after each tab's page load
> 
> Fixed telemetry.internal.backends.chrome.tab_list_backend_unittest.TabListBackendTest.testTabIdStableAfterTabCrash.
> If discarding a tab, TabMemoryMetricsReporter should not invoke EmitProcessMemoryMetrics for the tab because its process value is not valid.
> 
> TBR=tasak@google.com
> 
> (cherry picked from commit 006522629080281754763876f822bfb63550fa01)
> 
> Bug:  837498 , 846426
> Change-Id: Ic23f779b5d64eec90467494a2158c0d6e81ae4b2
> Reviewed-on: https://chromium-review.googlesource.com/1073109
> Commit-Queue: Takashi Sakamoto <tasak@google.com>
> Commit-Queue: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#561794}
> Reviewed-on: https://chromium-review.googlesource.com/1086669
> Reviewed-by: Takashi Sakamoto <tasak@google.com>
> Cr-Commit-Position: refs/branch-heads/3440@{#181}
> Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}

TBR=tasak@google.com,haraken@chromium.org

Change-Id: I846db6cf11095c6845362d823ab9c4a91aed5469
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  837498 , 846426, 849613
Reviewed-on: https://chromium-review.googlesource.com/1087329
Reviewed-by: Michael Moss <mmoss@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#201}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/5cc974fd9d3bee6cc3bee9ff6c296c4a9c00457a/chrome/browser/BUILD.gn
[modify] https://crrev.com/5cc974fd9d3bee6cc3bee9ff6c296c4a9c00457a/chrome/browser/resource_coordinator/tab_helper.cc
[delete] https://crrev.com/49e6eb80352f1137cb3a8fbd19723844e04778fe/chrome/browser/resource_coordinator/tab_memory_metrics_reporter.cc
[delete] https://crrev.com/49e6eb80352f1137cb3a8fbd19723844e04778fe/chrome/browser/resource_coordinator/tab_memory_metrics_reporter.h
[delete] https://crrev.com/49e6eb80352f1137cb3a8fbd19723844e04778fe/chrome/browser/resource_coordinator/tab_memory_metrics_reporter_unittest.cc
[modify] https://crrev.com/5cc974fd9d3bee6cc3bee9ff6c296c4a9c00457a/chrome/test/BUILD.gn

Project Member

Comment 15 by bugdroid1@chromium.org, Jun 7 2018

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

commit 73efcb115563045a4257b63ae2e6000fa9efe0ad
Author: Takashi Sakamoto <tasak@google.com>
Date: Thu Jun 07 04:54:29 2018

Reland [2/2] Report Memory_Experimental UKM 1, 5, 10 and 15 minutes after each tab's page load

Merge to release branch (branch-heads/3440).
Fixed telemetry.internal.backends.chrome.tab_list_backend_unittest.TabListBackendTest.testTabIdStableAfterTabCrash.
If discarding a tab, TabMemoryMetricsReporter should not invoke EmitProcessMemoryMetrics for the tab because its process value is not valid.

(cherry picked from commit 006522629080281754763876f822bfb63550fa01)

Bug:  837498 , 846426
Change-Id: I97bfe11d9157a9f050125d5919e1424f5a2c17f9
Reviewed-on: https://chromium-review.googlesource.com/1073109
Commit-Queue: Takashi Sakamoto <tasak@google.com>
Commit-Queue: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#561794}
Reviewed-on: https://chromium-review.googlesource.com/1088353
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#233}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/73efcb115563045a4257b63ae2e6000fa9efe0ad/chrome/browser/BUILD.gn
[modify] https://crrev.com/73efcb115563045a4257b63ae2e6000fa9efe0ad/chrome/browser/resource_coordinator/tab_helper.cc
[add] https://crrev.com/73efcb115563045a4257b63ae2e6000fa9efe0ad/chrome/browser/resource_coordinator/tab_memory_metrics_reporter.cc
[add] https://crrev.com/73efcb115563045a4257b63ae2e6000fa9efe0ad/chrome/browser/resource_coordinator/tab_memory_metrics_reporter.h
[add] https://crrev.com/73efcb115563045a4257b63ae2e6000fa9efe0ad/chrome/browser/resource_coordinator/tab_memory_metrics_reporter_unittest.cc
[modify] https://crrev.com/73efcb115563045a4257b63ae2e6000fa9efe0ad/chrome/test/BUILD.gn

Sign in to add a comment