New issue
Advanced search Search tips

Issue 896328 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 13
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

11.5%-1,076.5% regression in complex-iframe-filtered,tasks_per_frame_total_all,thread_raster_cpu_time_per_frame at 600016:600102

Project Member Reported by 42576172...@developer.gserviceaccount.com, Oct 17

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=896328

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


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

Android Nexus5X WebView Perf
mac-10_12_laptop_low_end-perf
mac-10_13_laptop_high_end-perf

blink_perf.paint - Benchmark documentation link:
  https://bit.ly/blink-perf-benchmarks

rendering.desktop - Benchmark documentation link:
  https://bit.ly/rendering-benchmarks
Cc: vmp...@chromium.org
Owner: vmp...@chromium.org
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/1113c52ae40000

Disable iframe isolation nodes via a flag. by vmpstr@chromium.org
https://chromium.googlesource.com/chromium/src/+/eeb3499ace08617926f1d0a7ece1c5f1ed677001
complex-iframe-filtered: 279.6 → 4334 (+4055)

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

Benchmark documentation link:
  https://bit.ly/blink-perf-benchmarks
Cc: chrishtr@chromium.org pdr@chromium.org
Between a rock and a hard place time:
- These regressions are due to disable iframe isolation nodes, meaning that it helps in raster time (probably due to fewer invalidations) and the test added specifically for this (complex-iframe-filtered)

However,
issue 895270 shows that the cost of these isolation nodes is cc memory regression. It's unclear to me why it would be cc memory regression other than the fact that we're not invalidating a part of the existing rasterized content so it shows up as a regression.

My initial thoughts is that I think the benefits outweigh the costs, but I wanted to check what everyone else thought about this.

+ericrk as well, do you know if the memory metrics on issue 895270 capture stable memory, invalidated or in flight one? Also do you know when the snapshot is actually taken?
Cc: wangxianzhu@chromium.org ericrk@chromium.org
ericrk, see #4
Based on the traces before and after the regression of issue 895270, the total "free_size" increased from 3680KiB to 8000KiB. The amount of increment seems the same as that of the total "effective_size". How should be interpret that?
I just talked with ericrk about this and here's the summary:
Basically the memory regression is in tiles that are pooled (ie, they are evictable if we just wait long enough), so that's why there's an increase in free size. The actual memory use by the page (ignoring pooled resources) is the same.

There is still a question of why the evictable resources is larger, so I'll try to repro this locally and just dump every time we're creating a tile. There's a chance that this is just a timing difference, but there's also a possibility that the patch somehow causes us to create different sized tiles temporarily.
 Issue 896827  has been merged into this issue.
I've run this locally and the memory usage between the two runs is different, although with my local build the memory usage is slightly flipped (ie enabling the flag causes a lower memory usage).

I also logged all of the tile sizes and there's a ton of tiles on this page... From the brief look, I don't really see anything particularly wrong, some tiles are created before others with the same sizes.

I'm going to reland the patch.
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 19

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

commit 62f7e22563e1670204d6960af0c54558b4f0fb7d
Author: Vladimir Levin <vmpstr@chromium.org>
Date: Fri Oct 19 21:27:16 2018

Re-enable LayoutViewIsolationNodes.

This patch re-enables isolation nodes for layout views (iframes).

It seems that this causes a memory regression on one of the telemetry
tests, but it seems that it might be a timing issue. The evidence for
this is that the regression happens in free memory (evictable) and
the tile sizes seem to be same or similar with or without the flag.

For more details, see the referenced bug.

R=chrishtr@chromium.org, pdr@chromium.org, wangxianzhu@chromium.org

Bug:  896328 
Change-Id: I9c399f253f246364b68980350cec3d14e5690b75
Reviewed-on: https://chromium-review.googlesource.com/c/1292511
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Commit-Queue: vmpstr <vmpstr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601301}
[modify] https://crrev.com/62f7e22563e1670204d6960af0c54558b4f0fb7d/third_party/blink/renderer/platform/runtime_enabled_features.json5

Status: Fixed (was: Assigned)

Sign in to add a comment