New issue
Advanced search Search tips
Starred by 16 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android , Windows , Mac
Pri: 2
Type: Bug

Blocked on:
issue 907067

Blocking:
issue 839084



Sign in to add a comment
link

Issue 888558: Performance changes related to enabling Perfetto by default for all telemetry tests

Reported by pasko@chromium.org, Sep 24 Project Member

Issue description

There are a few regressions and improvements. Some of them are surely false alarms. Perfetto was recently reverted, so those alerts are no longer release blocking.

Let's resolve these alerts as Duplicate for this bug. This way we can collect all the alerts in one place.

Fixing this bug would mean going through all the performance changes that are marked-as-duplicate and either proving that the effect is expected or preventing the perf change for the next Perfetto enabling (or keeping the legitimate improvement:P)
 

Comment 1 by pasko@chromium.org, Sep 24

Summary: Performance changes related to enabling Perfetto by default for all telemetry tests (was: Performance changes Enabling Perfetto by default for all telemetry tests)

Comment 2 by pasko@chromium.org, Sep 24

Cc: kouhei@chromium.org pasko@chromium.org
 Issue 888521  has been merged into this issue.

Comment 3 by pasko@chromium.org, Sep 24

Issue 887567 has been merged into this issue.

Comment 4 by pasko@chromium.org, Sep 24

Cc: alexclarke@chromium.org
 Issue 887486  has been merged into this issue.

Comment 5 by pasko@chromium.org, Sep 24

Cc: oysteine@chromium.org
 Issue 887475  has been merged into this issue.

Comment 6 by pasko@chromium.org, Sep 24

Issue 887375 has been merged into this issue.

Comment 7 by pasko@chromium.org, Sep 24

 Issue 887359  has been merged into this issue.

Comment 8 by pasko@chromium.org, Sep 24

 Issue 888549  has been merged into this issue.

Comment 9 by pasko@chromium.org, Sep 24

 Issue 888543  has been merged into this issue.

Comment 10 by pasko@chromium.org, Sep 24

Issue 887301 has been merged into this issue.

Comment 11 by pasko@chromium.org, Sep 24

Issue 887291 has been merged into this issue.

Comment 12 by pasko@chromium.org, Sep 24

Issue 887279 has been merged into this issue.

Comment 13 by pasko@chromium.org, Sep 24

Issue 887278 has been merged into this issue.

Comment 14 by pasko@chromium.org, Sep 24

Issue 887338 has been merged into this issue.

Comment 15 by pasko@chromium.org, Sep 24

Cc: oysteine@google.com maxlg@chromium.org
 Issue 887335  has been merged into this issue.

Comment 16 by pasko@chromium.org, Sep 24

 Issue 887329  has been merged into this issue.

Comment 17 by pasko@chromium.org, Sep 24

 Issue 887327  has been merged into this issue.

Comment 18 by pasko@chromium.org, Sep 24

 Issue 887319  has been merged into this issue.

Comment 19 by pasko@chromium.org, Sep 24

 Issue 887302  has been merged into this issue.

Comment 20 by pasko@chromium.org, Sep 24

Issue 887249 has been merged into this issue.

Comment 21 by pasko@chromium.org, Sep 24

 Issue 886890  has been merged into this issue.

Comment 22 by pasko@chromium.org, Sep 24

 Issue 886888  has been merged into this issue.

Comment 23 by pasko@chromium.org, Sep 24

 Issue 886786  has been merged into this issue.

Comment 24 by pasko@chromium.org, Sep 24

 Issue 886891  has been merged into this issue.

Comment 25 by pasko@chromium.org, Sep 24

 Issue 886785  has been merged into this issue.

Comment 26 by pasko@chromium.org, Sep 24

 Issue 886784  has been merged into this issue.

Comment 27 by pasko@chromium.org, Sep 24

 Issue 886773  has been merged into this issue.

Comment 28 by pasko@chromium.org, Sep 24

Issue 886686 has been merged into this issue.

Comment 29 by pasko@chromium.org, Sep 24

 Issue 886683  has been merged into this issue.

Comment 30 by pasko@chromium.org, Sep 24

Issue 886677 has been merged into this issue.

Comment 31 by pasko@chromium.org, Sep 25

Issue 888645 has been merged into this issue.

Comment 32 by dalecur...@chromium.org, Sep 25

Issue 888646 has been merged into this issue.

Comment 33 by oysteine@chromium.org, Sep 25

Thanks for all of the triaging work, pasko@! It's super helpful in figuring out necessary steps before we attempt landing this again.

Comment 34 by pasko@chromium.org, Sep 26

Cc: treib@chromium.org lanwei@chromium.org blundell@chromium.org
 Issue 889171  has been merged into this issue.

Comment 35 by pasko@chromium.org, Sep 26

re #33: sure, great to hear that it will help. With the new pinpoint doing so is easy, it also displays information quickly, unlike other internal tools :)

Comment 36 by eseckler@chromium.org, Oct 4

Components: Speed>Tracing

Comment 37 by eseckler@chromium.org, Oct 4

Cc: ushesh@chromium.org
 Issue 883521  has been merged into this issue.

Comment 38 by eseckler@chromium.org, Oct 4

 Issue 883524  has been merged into this issue.

Comment 39 by eseckler@chromium.org, Oct 4

Issue 880455 has been merged into this issue.

Comment 40 by eseckler@chromium.org, Oct 4

Issue 880300 has been merged into this issue.

Comment 41 by eseckler@chromium.org, Oct 24

FYI, the memory regressions on Windows (e.g. crbug.com/886686) should be addressed by https://android-review.googlesource.com/c/platform/external/perfetto/+/799400.

Comment 42 by oysteine@google.com, Oct 31

With the above and https://chromium-review.googlesource.com/c/chromium/src/+/1263354 the regressions should be addressed, though it's possible there's still some regressions in sensitive microbenchmarks.

Re-enabled Perfetto for Telemetry in https://chromium-review.googlesource.com/c/chromium/src/+/1307793

Comment 43 by oysteine@chromium.org, Nov 2

Another fix landed, for worker thread events going missing: https://chromium-review.googlesource.com/c/chromium/src/+/1311125 

One interesting side-effect of the above bugfix is that it makes it pretty clear what a large impact the lock each trace event had to grab on worker threads, prior to Perfetto, had on very sensitive microbenchmarks: https://chromeperf.appspot.com/report?sid=e1969fad0ec7f5437927cf341895223ef1ada3de7d0c32994434cda042cff8ee&start_rev=602020&end_rev=604975

I'll keep duping perf bugs to this parent bug. Nothing too concerning has popped up as of yet; meaning nothing that would cause us to miss future regressions due to highly increased measurement overhead.

Mainly:
* rendering.mobile/tasks_per_frame_total_all goes up by 3-10%.
* memory:webview:all_processes:reported_by_chrome:malloc:effective_size going up by 400kB (presumably the shared memory buffer we use).
* rendering.mobile/queuing_durations going up by 10-30% (I have no idea what this actually measures but I'm investigating)
rendering.mobile/thread_raster_cpu_time_per_frame both improves and regresses by decent amounts (the above-mentioned sensitive metric).

Comment 44 by oysteine@chromium.org, Nov 2

Cc: hjd@google.com
 Issue 900927  has been merged into this issue.

Comment 45 by oysteine@chromium.org, Nov 2

Issue 901360 has been merged into this issue.

Comment 46 by oysteine@chromium.org, Nov 2

Issue 901365 has been merged into this issue.

Comment 47 by oysteine@chromium.org, Nov 2

Cc: tdres...@chromium.org
 Issue 901367  has been merged into this issue.

Comment 48 by oysteine@chromium.org, Nov 2

 Issue 901027  has been merged into this issue.

Comment 49 by oysteine@chromium.org, Nov 2

 Issue 900884  has been merged into this issue.

Comment 50 by oysteine@chromium.org, Nov 2

 Issue 900886  has been merged into this issue.

Comment 51 by oysteine@chromium.org, Nov 2

 Issue 901364  has been merged into this issue.

Comment 52 by oysteine@chromium.org, Nov 2

 Issue 901359  has been merged into this issue.

Comment 53 by oysteine@chromium.org, Nov 2

 Issue 901356  has been merged into this issue.

Comment 54 by oysteine@chromium.org, Nov 2

 Issue 901358  has been merged into this issue.

Comment 55 by oysteine@chromium.org, Nov 2

 Issue 901354  has been merged into this issue.

Comment 56 by oysteine@chromium.org, Nov 2

Issue 901370 has been merged into this issue.

Comment 57 by oysteine@chromium.org, Nov 5

Cc: dougman@google.com chrome-release-bot@chromium.org
 Issue 901453  has been merged into this issue.

Comment 58 by oysteine@chromium.org, Nov 5

Issue 901918 has been merged into this issue.

Comment 59 by oysteine@chromium.org, Nov 5

Issue 901916 has been merged into this issue.

Comment 60 by blundell@chromium.org, Nov 6

Cc: -blundell@chromium.org

Comment 61 by oysteine@chromium.org, Nov 7

Cc: chiniforooshan@chromium.org
 Issue 901682  has been merged into this issue.

Comment 62 by oysteine@chromium.org, Nov 7

Issue 902525 has been merged into this issue.

Comment 63 by oysteine@chromium.org, Nov 13

 Issue 904887  has been merged into this issue.

Comment 64 by oysteine@chromium.org, Nov 13

Issue 901890 has been merged into this issue.

Comment 65 by oysteine@chromium.org, Nov 13

 Issue 903133  has been merged into this issue.

Comment 66 by eseckler@chromium.org, Nov 19

Most of these changes in metrics, especially thread_time* and tasks_per_frame*, seem to be due to differences in the covered threads in the traces.

For example (from https://chromeperf.appspot.com/group_report?bug_id=903133):
before: https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/js_poster_circle_2018-10-29_19-23-04_35892.html
after:  https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/js_poster_circle_2018-11-01_09-14-23_69415.html

Perfetto seems to be missing some TaskSchedulerForegroundWorker threads in the renderer, but includes a TaskSchedulerBackgroundWorker which TraceLog is missing.

Oystein, any idea why that might be?

Comment 67 by eseckler@chromium.org, Nov 20

Blockedon: 907067

Comment 68 by bugdroid1@chromium.org, Nov 29

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/53e8b2a294d66219b5c4daf00dda1cb9f822a54f

commit 53e8b2a294d66219b5c4daf00dda1cb9f822a54f
Author: Oystein Eftevaag <oysteine@google.com>
Date: Thu Nov 29 21:32:46 2018

Perfetto: Stop splitting _COMPLETE events

The current Perfetto backend splits _COMPLETE trace events into
separate _BEGIN and _END pairs, as it's not very feasible to modify
existing events after they're written into the Shared Memory Buffers.

This is causing some issues with the trace-viewer which has some
assumptions about the ordering of begin/end events vs. async events,
and is also bloating the sizes of traces and adding extra
overhead for the perf infra.

Instead, we now keep the _COMPLETE events in an internal stack in
TLS and only emit them when we have their duration.

R=eseckler@chromium.org,skyostil@chromium.org

Bug: 909728, 888558 
Change-Id: I80e37264de66d8bbcb6c9095d21047957fd6eb9f
Reviewed-on: https://chromium-review.googlesource.com/c/1354503
Commit-Queue: oysteine <oysteine@chromium.org>
Reviewed-by: Eric Seckler <eseckler@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612360}
[modify] https://crrev.com/53e8b2a294d66219b5c4daf00dda1cb9f822a54f/base/trace_event/trace_log.cc
[modify] https://crrev.com/53e8b2a294d66219b5c4daf00dda1cb9f822a54f/base/trace_event/trace_log.h
[modify] https://crrev.com/53e8b2a294d66219b5c4daf00dda1cb9f822a54f/base/trace_event/traced_value.cc
[modify] https://crrev.com/53e8b2a294d66219b5c4daf00dda1cb9f822a54f/services/tracing/public/cpp/perfetto/trace_event_data_source.cc
[modify] https://crrev.com/53e8b2a294d66219b5c4daf00dda1cb9f822a54f/services/tracing/public/cpp/perfetto/trace_event_data_source.h
[modify] https://crrev.com/53e8b2a294d66219b5c4daf00dda1cb9f822a54f/services/tracing/public/cpp/perfetto/trace_event_data_source_unittest.cc

Comment 70 by bugdroid1@chromium.org, Nov 30

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6a796b426b1ff987c8e4fd63d838731bcfd50d54

commit 6a796b426b1ff987c8e4fd63d838731bcfd50d54
Author: oysteine <oysteine@chromium.org>
Date: Fri Nov 30 20:55:56 2018

Revert "Perfetto: Stop splitting _COMPLETE events"

This reverts commit 53e8b2a294d66219b5c4daf00dda1cb9f822a54f.

Reason for revert:  crbug.com/910704 

Original change's description:
> Perfetto: Stop splitting _COMPLETE events
> 
> The current Perfetto backend splits _COMPLETE trace events into
> separate _BEGIN and _END pairs, as it's not very feasible to modify
> existing events after they're written into the Shared Memory Buffers.
> 
> This is causing some issues with the trace-viewer which has some
> assumptions about the ordering of begin/end events vs. async events,
> and is also bloating the sizes of traces and adding extra
> overhead for the perf infra.
> 
> Instead, we now keep the _COMPLETE events in an internal stack in
> TLS and only emit them when we have their duration.
> 
> R=​eseckler@chromium.org,skyostil@chromium.org
> 
> Bug: 909728, 888558 
> Change-Id: I80e37264de66d8bbcb6c9095d21047957fd6eb9f
> Reviewed-on: https://chromium-review.googlesource.com/c/1354503
> Commit-Queue: oysteine <oysteine@chromium.org>
> Reviewed-by: Eric Seckler <eseckler@chromium.org>
> Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#612360}

TBR=oysteine@chromium.org,skyostil@chromium.org,eseckler@chromium.org

Change-Id: I0cda9ab36248042c3889d810bfd07a6d03a6d58e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 909728,  888558 
Reviewed-on: https://chromium-review.googlesource.com/c/1357500
Reviewed-by: oysteine <oysteine@chromium.org>
Commit-Queue: oysteine <oysteine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612776}
[modify] https://crrev.com/6a796b426b1ff987c8e4fd63d838731bcfd50d54/base/trace_event/trace_log.cc
[modify] https://crrev.com/6a796b426b1ff987c8e4fd63d838731bcfd50d54/base/trace_event/trace_log.h
[modify] https://crrev.com/6a796b426b1ff987c8e4fd63d838731bcfd50d54/base/trace_event/traced_value.cc
[modify] https://crrev.com/6a796b426b1ff987c8e4fd63d838731bcfd50d54/services/tracing/public/cpp/perfetto/trace_event_data_source.cc
[modify] https://crrev.com/6a796b426b1ff987c8e4fd63d838731bcfd50d54/services/tracing/public/cpp/perfetto/trace_event_data_source.h
[modify] https://crrev.com/6a796b426b1ff987c8e4fd63d838731bcfd50d54/services/tracing/public/cpp/perfetto/trace_event_data_source_unittest.cc

Comment 71 by bugdroid1@chromium.org, Dec 7

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9d830c214cd1c59958ee19403b446fa5b796e794

commit 9d830c214cd1c59958ee19403b446fa5b796e794
Author: Oystein Eftevaag <oysteine@google.com>
Date: Fri Dec 07 01:16:41 2018

Reland "Perfetto: Stop splitting _COMPLETE events"

This is a reland of 53e8b2a294d66219b5c4daf00dda1cb9f822a54f

Relanding after https://chromium-review.googlesource.com/c/catapult/+/1364251
which is what caused the failures triggering the revert last time.

Original change's description:
> Perfetto: Stop splitting _COMPLETE events
>
> The current Perfetto backend splits _COMPLETE trace events into
> separate _BEGIN and _END pairs, as it's not very feasible to modify
> existing events after they're written into the Shared Memory Buffers.
>
> This is causing some issues with the trace-viewer which has some
> assumptions about the ordering of begin/end events vs. async events,
> and is also bloating the sizes of traces and adding extra
> overhead for the perf infra.
>
> Instead, we now keep the _COMPLETE events in an internal stack in
> TLS and only emit them when we have their duration.
>
TBR=eseckler@chromium.org,skyostil@chromium.org
>
> Bug: 909728, 888558 
> Change-Id: I80e37264de66d8bbcb6c9095d21047957fd6eb9f
> Reviewed-on: https://chromium-review.googlesource.com/c/1354503
> Commit-Queue: oysteine <oysteine@chromium.org>
> Reviewed-by: Eric Seckler <eseckler@chromium.org>
> Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#612360}

Bug: 909728,  888558 
Change-Id: I5e3d6e0f170066011e2a5b452969d9c8cd18ac4f
Reviewed-on: https://chromium-review.googlesource.com/c/1359304
Reviewed-by: oysteine <oysteine@chromium.org>
Commit-Queue: oysteine <oysteine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614540}
[modify] https://crrev.com/9d830c214cd1c59958ee19403b446fa5b796e794/base/trace_event/trace_log.cc
[modify] https://crrev.com/9d830c214cd1c59958ee19403b446fa5b796e794/base/trace_event/trace_log.h
[modify] https://crrev.com/9d830c214cd1c59958ee19403b446fa5b796e794/base/trace_event/traced_value.cc
[modify] https://crrev.com/9d830c214cd1c59958ee19403b446fa5b796e794/services/tracing/public/cpp/perfetto/trace_event_data_source.cc
[modify] https://crrev.com/9d830c214cd1c59958ee19403b446fa5b796e794/services/tracing/public/cpp/perfetto/trace_event_data_source.h
[modify] https://crrev.com/9d830c214cd1c59958ee19403b446fa5b796e794/services/tracing/public/cpp/perfetto/trace_event_data_source_unittest.cc

Comment 72 by oysteine@chromium.org, Dec 11

Status: Fixed (was: Assigned)
Looks like #71 is sticking now and made the graphs return to normal levels.

Sign in to add a comment