Issue metadata
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 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Oct 17
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/1113c52ae40000
,
Oct 17
📍 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
,
Oct 18
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?
,
Oct 18
ericrk, see #4
,
Oct 18
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?
,
Oct 18
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.
,
Oct 18
Issue 896827 has been merged into this issue.
,
Oct 19
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.
,
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
,
Nov 13
|
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Oct 17