New issue
Advanced search Search tips

Issue 888222 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 30
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: ----



Sign in to add a comment

loading benchmarks are failing everywhere

Project Member Reported by sheriff-...@appspot.gserviceaccount.com, Sep 22

Issue description

Cc: oysteine@chromium.org
Owner: oysteine@chromium.org
Status: Assigned (was: Available)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/117b92b8e40000

Perfetto: Fix trace buffer memory size by oysteine@chromium.org
https://chromium.googlesource.com/chromium/src/+/c2747b15b2ba943737eb077e0f6783b3d0710633
0 → 1 (+1)

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions
Reverting. For the record, an example failed build is https://ci.chromium.org/p/chrome/builders/luci.chrome.ci/win-10-perf/674
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 22

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

commit 907ce2cc8ae97bfaee70a924552b2b66a0c67667
Author: Ned Nguyen <nednguyen@google.com>
Date: Sat Sep 22 09:36:59 2018

Revert "Perfetto: Fix trace buffer memory size"

This reverts commit c2747b15b2ba943737eb077e0f6783b3d0710633.

Reason for revert: breaking many loading perf tests 

BUG:888222

Original change's description:
> Perfetto: Fix trace buffer memory size
> 
> Some unnoticed debug sizing caused the host-side tracing buffer
> to be a bit larger than necessary.
> 
> R=​ssid@chromium.org
> 
> Bug: 886685
> Change-Id: I56f3ed15952d7f0d6382b6e939c5386169f672b8
> Reviewed-on: https://chromium-review.googlesource.com/1236663
> Reviewed-by: Siddhartha S <ssid@chromium.org>
> Commit-Queue: oysteine <oysteine@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#593257}

TBR=oysteine@chromium.org,ssid@chromium.org

Change-Id: I6a19cad20042d06bc6ac8406cb1f8ac9d3fa79ca
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 886685
Reviewed-on: https://chromium-review.googlesource.com/1239077
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Ned Nguyen <nednguyen@google.com>
Cr-Commit-Position: refs/heads/master@{#593440}
[modify] https://crrev.com/907ce2cc8ae97bfaee70a924552b2b66a0c67667/services/tracing/perfetto/json_trace_exporter.cc
[modify] https://crrev.com/907ce2cc8ae97bfaee70a924552b2b66a0c67667/services/tracing/perfetto/perfetto_integration_unittest.cc

Components: Speed>Tracing
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 25

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

commit 1851c4fc238c0d86250aeb33206348eaa76caf04
Author: Eric Seckler <eseckler@chromium.org>
Date: Thu Oct 25 12:19:19 2018

perfetto: Change trace buffer size to 32mB

Previously attempted to set it to 4mB in crrev.com/c/1236663, but that
caused some flakiness in benchmarks. So try it again with 32mB.

Note that as of [1], this buffer is committed only as it is accessed
even on Windows, so the memory regressions we were seeing in
crbug.com/886686 should be much more limited now anyway.

[1] https://android-review.googlesource.com/c/platform/external/perfetto/+/799400

Bug: 839071,  888222 
Change-Id: If7108f3bc0b226e132087886aa8cbccbc30e79b6
Reviewed-on: https://chromium-review.googlesource.com/c/1297418
Commit-Queue: Eric Seckler <eseckler@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: oysteine <oysteine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602675}
[modify] https://crrev.com/1851c4fc238c0d86250aeb33206348eaa76caf04/services/tracing/perfetto/json_trace_exporter.cc
[modify] https://crrev.com/1851c4fc238c0d86250aeb33206348eaa76caf04/services/tracing/perfetto/perfetto_integration_unittest.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 27

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

commit faa1cd30772dee754ebda1d8a1841627d64c543b
Author: Christian Biesinger <cbiesinger@chromium.org>
Date: Sat Oct 27 06:44:31 2018

Revert "perfetto: Change trace buffer size to 32mB"

This reverts commit 1851c4fc238c0d86250aeb33206348eaa76caf04.

Reason for revert: this may have broken the layoutng loading.desktop perf tests: https://ci.chromium.org/p/chrome/builders/luci.chrome.ci/linux-perf-fyi/244

Original change's description:
> perfetto: Change trace buffer size to 32mB
> 
> Previously attempted to set it to 4mB in crrev.com/c/1236663, but that
> caused some flakiness in benchmarks. So try it again with 32mB.
> 
> Note that as of [1], this buffer is committed only as it is accessed
> even on Windows, so the memory regressions we were seeing in
> crbug.com/886686 should be much more limited now anyway.
> 
> [1] https://android-review.googlesource.com/c/platform/external/perfetto/+/799400
> 
> Bug: 839071,  888222 
> Change-Id: If7108f3bc0b226e132087886aa8cbccbc30e79b6
> Reviewed-on: https://chromium-review.googlesource.com/c/1297418
> Commit-Queue: Eric Seckler <eseckler@chromium.org>
> Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
> Reviewed-by: oysteine <oysteine@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#602675}

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

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

Bug: 839071,  888222 
Change-Id: Idd0feff4c3164635609e0e0851e8f679889b367c
Reviewed-on: https://chromium-review.googlesource.com/c/1303676
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603316}
[modify] https://crrev.com/faa1cd30772dee754ebda1d8a1841627d64c543b/services/tracing/perfetto/json_trace_exporter.cc
[modify] https://crrev.com/faa1cd30772dee754ebda1d8a1841627d64c543b/services/tracing/perfetto/perfetto_integration_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment