Issue 828026 link

Starred by 1 user

Issue metadata

Status:	Assigned
Owner:	ericrk@chromium.org
Cc:	xiaoche...@chromium.org █ groby@chromium.org erikc...@chromium.org
EstimatedDays:	----
NextAction:	----
OS:	----
Pri:	2
Type:	Bug-Regression
Performance-Memory Performance-Sheriff M-67

13.6%-91.5% regression in system_health.memory_desktop at 547232:547380

Project Member Reported by pmeenan@chromium.org, Apr 2 2018

Issue description

See the link to graphs below.

Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Apr 2 2018

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

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


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

chromium-rel-mac11-air
chromium-rel-mac11-pro
chromium-rel-mac12
chromium-rel-win10
win-high-dpi

Project Member

Comment 2 by 42576172...@developer.gserviceaccount.com, Apr 2 2018

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/15c6e00f440000

Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Apr 2 2018

Cc: tkent@chromium.org groby@chromium.org xiaoche...@chromium.org
Owner: xiaoche...@chromium.org
Status: Assigned (was: Untriaged)

📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/15c6e00f440000

Make CapurePageText capture textContent only on dirty layout by xiaochengh@chromium.org
https://chromium.googlesource.com/chromium/src/+/802df0571195f064c2bd77b513b86950eeb9f52a

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 3 2018

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

commit f397777d8c4e6855f580f0f2d1a1ddd9b0eec217
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Tue Apr 03 19:53:06 2018

Revert "Make CapurePageText capture textContent only on dirty layout"

This reverts commit 802df0571195f064c2bd77b513b86950eeb9f52a.

Reason for revert: suspected for causing memory regressions crbug.com/828026 and  crbug.com/828027 

Original change's description:
> Make CapurePageText capture textContent only on dirty layout
> 
> This is a partial revert for r536969 Change TranslateHelper to use a
> TextContent-like text dump.
> 
> r536969 is a workaround for a lifecycle violation bug that layout is
> still dirty after running layout update.
> 
> It turns out that textContent is not a good text dump for
> TranslateHelper, as pages may hide arbitrary content (e.g., JSON) in
> invisible text nodes, which can't be reliably filtered without checking
> element style or layout.
> 
> Since we don't always run CapturePageText() with dirty layout, there is
> no need to always dump textContent. Hence, this patch partially reverts
> r536969 that, we still dump innerText as long as layout is clean. We
> dump textContent only when layout is dirty, which is enough to get
> around the lifecycle violation bug.
> 
> Bug:  826174 
> Change-Id: I63e97832caa9858e67992847fb9a22f6405183cb
> Reviewed-on: https://chromium-review.googlesource.com/984474
> Reviewed-by: Kent Tamura <tkent@chromium.org>
> Reviewed-by: Rachel Blum <groby@chromium.org>
> Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#547334}

TBR=groby@chromium.org,tkent@chromium.org,xiaochengh@chromium.org

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

Bug:  826174 , 828026,  828027 
Change-Id: I4c3200e5c7de0e6b3838ddcc40d53e751538f915
Reviewed-on: https://chromium-review.googlesource.com/992818
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547796}
[modify] https://crrev.com/f397777d8c4e6855f580f0f2d1a1ddd9b0eec217/third_party/WebKit/Source/core/exported/WebFrameContentDumper.cpp

Comment 5 by xiaoche...@chromium.org, Apr 4 2018

Owner: ----
Status: Untriaged (was: Assigned)

I've reverted all my relevant changes:

chromium / chromium / src / 6573295f56c6cc83ecdbdb1f4cc4b7ac64882775 / third_party / WebKit / Source / core / exported / WebFrameContentDumper.cpp

98199e90 Revert "Change TranslateHelper to use a textContent-like text dump" by Xiaocheng Hu · 13 hours ago
ccfb59a Revert "Eliminate NOSCRIPT element from TranslateHelper text dump" by Xiaocheng Hu · 17 hours ago
f397777 Revert "Make CapurePageText capture textContent only on dirty layout" by Xiaocheng Hu · 23 hours ago
802df05 Make CapurePageText capture textContent only on dirty layout by Xiaocheng Hu · 5 days ago
797a380 Eliminate NOSCRIPT element from TranslateHelper text dump by Xiaocheng Hu · 7 weeks ago
605ee93 Change TranslateHelper to use a textContent-like text dump by Xiaocheng Hu · 7 weeks ago

However, the regression is still there.

Might be some other bug revealed by my change 802df05? I don't have a good idea. Asking for re-triage.

Comment 6 by xiaoche...@chromium.org, Apr 4 2018

 Issue 828027  has been merged into this issue.

Comment 7 by sullivan@google.com, May 3 2018

Owner: erikc...@chromium.org
Status: Assigned (was: Untriaged)

Erik, can you help triage? The bisect showed a pretty clear regression at that CL, but reverting didn't help. Kicking off another bisect, but any ideas what could be going on?

Project Member

Comment 8 by 42576172...@developer.gserviceaccount.com, May 3 2018

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/13ff2fcbc40000

Project Member

Comment 9 by 42576172...@developer.gserviceaccount.com, May 3 2018

📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/13ff2fcbc40000

Make CapurePageText capture textContent only on dirty layout by xiaochengh@chromium.org
https://chromium.googlesource.com/chromium/src/+/802df0571195f064c2bd77b513b86950eeb9f52a

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Comment 10 by erikc...@chromium.org, May 4 2018

Cc: erikc...@chromium.org
Owner: ericrk@chromium.org

The difference is caused by the fact that we're creating [and not destroying] an additional GPU buffer. 

There was a drop in the metric on the run that first included the revert. But then the metric rose again. It will be hard to figure out what's going on without knowing more about what's going on. Over to ericrk.