New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 860297 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 9
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 854212



Sign in to add a comment

Deprecate and remove timeline_model.GetRendererThreadFromTabId

Project Member Reported by perezju@chromium.org, Jul 4

Issue description

This method does not work well in some situations (c.f.  issue 854212 ), and won't be needed after timeline_model is finally deprecated.

For the moment we can just replace this with a simpler "GetFirstRendererThread" to support the use cases of legacy benchmarks still relying on timeline_model.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 5

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/06b0746bd49c929814769a3f1a625a0de18e0ebd

commit 06b0746bd49c929814769a3f1a625a0de18e0ebd
Author: Juan Antonio Navarro Perez <perezju@chromium.org>
Date: Thu Jul 05 08:35:26 2018

[Telemetry] Add tab.AddTimelineMarker method

This is a pretty common operation both for testing and during normal
Telemetry execution.

Bug:  chromium:860297 
Change-Id: I6e4aef9c51bef0d6692fbbeb57a572bbeac3208e
Reviewed-on: https://chromium-review.googlesource.com/1126380
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>

[modify] https://crrev.com/06b0746bd49c929814769a3f1a625a0de18e0ebd/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py
[modify] https://crrev.com/06b0746bd49c929814769a3f1a625a0de18e0ebd/telemetry/telemetry/internal/backends/chrome_inspector/inspector_backend.py
[modify] https://crrev.com/06b0746bd49c929814769a3f1a625a0de18e0ebd/telemetry/telemetry/internal/browser/tab_unittest.py
[modify] https://crrev.com/06b0746bd49c929814769a3f1a625a0de18e0ebd/telemetry/telemetry/core/tracing_controller_unittest.py
[modify] https://crrev.com/06b0746bd49c929814769a3f1a625a0de18e0ebd/telemetry/telemetry/internal/browser/web_contents.py
[modify] https://crrev.com/06b0746bd49c929814769a3f1a625a0de18e0ebd/telemetry/telemetry/internal/platform/tracing_agent/chrome_tracing_agent.py
[modify] https://crrev.com/06b0746bd49c929814769a3f1a625a0de18e0ebd/telemetry/telemetry/page/cache_temperature.py

Project Member

Comment 2 by bugdroid1@chromium.org, Jul 5

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/4746e9702ae7d81063ab04053836ad17c45f5641

commit 4746e9702ae7d81063ab04053836ad17c45f5641
Author: Juan Antonio Navarro Perez <perezju@chromium.org>
Date: Thu Jul 05 08:38:06 2018

[Telemetry] Fix logic in Tab.testGetRendererThreadFromTabId

The test was previously asserting the trivial statement that a list
with a single element on it (which happened to be an iterator) had
exactly one element.

Actually unrolling the iterator showed that it was in fact empty,
probably because the logic to find timeline markers changed at some
point and this broken test failed to detect it.

This CL now:
- Adds the necessary logic to a new thread.IterTimelineMarkers,
- Implements model.FindTimelineMarkers in terms of this new method,
- and fixes the assertion in the test.

Bug:  chromium:860297 
Change-Id: I84213896c0da97c5e599c6bae342b6af7a3b7527
Reviewed-on: https://chromium-review.googlesource.com/1126306
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>

[modify] https://crrev.com/4746e9702ae7d81063ab04053836ad17c45f5641/telemetry/telemetry/timeline/model.py
[modify] https://crrev.com/4746e9702ae7d81063ab04053836ad17c45f5641/telemetry/telemetry/timeline/event_container.py
[modify] https://crrev.com/4746e9702ae7d81063ab04053836ad17c45f5641/telemetry/telemetry/internal/browser/tab_unittest.py

Project Member

Comment 3 by bugdroid1@chromium.org, Jul 5

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

commit e49cbaab69f8ea8787f82e134506d419c7f1f84a
Author: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Thu Jul 05 10:15:17 2018

Roll src/third_party/catapult 661bab1b6bb9..06b0746bd49c (1 commits)

https://chromium.googlesource.com/catapult.git/+log/661bab1b6bb9..06b0746bd49c


git log 661bab1b6bb9..06b0746bd49c --date=short --no-merges --format='%ad %ae %s'
2018-07-05 perezju@chromium.org [Telemetry] Add tab.AddTimelineMarker method


Created with:
  gclient setdep -r src/third_party/catapult@06b0746bd49c

The AutoRoll server is located here: https://catapult-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG= chromium:860297 
TBR=sullivan@chromium.org

Change-Id: I9e71b9a9bf0fcd52b12394c0b7bceb515d5a1f4a
Reviewed-on: https://chromium-review.googlesource.com/1126782
Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#572749}
[modify] https://crrev.com/e49cbaab69f8ea8787f82e134506d419c7f1f84a/DEPS

After a bit of thinking I'm now considering to make the API

    model.GetFirstRendererThread(tab_id)

which both find the thread marked as "first-renderer" (normally the one in foreground when the trace is collected) *and* verify that this thread also includes the matching tab_id.

This makes it a drop in replacement of the existing:

    model.GetRendererThreadFromTabId(tab_id)

And will complain loudly if we accidentally retrieve the wrong renderer that a TBMv1 measurement was expecting.
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 5

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

commit 60cf0ab45b30f3de672f4da55e2907976a925d27
Author: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Thu Jul 05 11:58:43 2018

Roll src/third_party/catapult 06b0746bd49c..4746e9702ae7 (1 commits)

https://chromium.googlesource.com/catapult.git/+log/06b0746bd49c..4746e9702ae7


git log 06b0746bd49c..4746e9702ae7 --date=short --no-merges --format='%ad %ae %s'
2018-07-05 perezju@chromium.org [Telemetry] Fix logic in Tab.testGetRendererThreadFromTabId


Created with:
  gclient setdep -r src/third_party/catapult@4746e9702ae7

The AutoRoll server is located here: https://catapult-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG= chromium:860297 
TBR=sullivan@chromium.org

Change-Id: I699d8d4a4ef4a79b52f0bef50d019660dde72dcc
Reviewed-on: https://chromium-review.googlesource.com/1126979
Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#572761}
[modify] https://crrev.com/60cf0ab45b30f3de672f4da55e2907976a925d27/DEPS

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 5

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

commit 24e22c7773f2aabdde32b823f177c67869980639
Author: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Thu Jul 05 14:22:25 2018

Roll src/third_party/catapult 4746e9702ae7..8355b184a56e (1 commits)

https://chromium.googlesource.com/catapult.git/+log/4746e9702ae7..8355b184a56e


git log 4746e9702ae7..8355b184a56e --date=short --no-merges --format='%ad %ae %s'
2018-07-05 perezju@chromium.org [Telemetry] Add model.GetFirstRendererThread() method


Created with:
  gclient setdep -r src/third_party/catapult@8355b184a56e

The AutoRoll server is located here: https://catapult-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG= chromium:860297 
TBR=sullivan@chromium.org

Change-Id: I44bc180dcb4d1835934b57d9534d156d49fb56da
Reviewed-on: https://chromium-review.googlesource.com/1127059
Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#572783}
[modify] https://crrev.com/24e22c7773f2aabdde32b823f177c67869980639/DEPS

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 6

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

commit f0909821f41ff79e3a7a650cabd94e7d9db24257
Author: Juan Antonio Navarro Perez <perezju@chromium.org>
Date: Fri Jul 06 09:44:54 2018

[gpu_tests] Switch to timeline_model.IsSliceOrAsyncSlice

Small Telemetry refactor: The helper function IsSliceOrAsyncSlice is
being moved from the module to a static method of the class itself.

Depends on:
https://chromium-review.googlesource.com/c/catapult/+/1126306

Bug:  860297 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: If54f32536a965d8a7d8f094bf31806106b092015
Reviewed-on: https://chromium-review.googlesource.com/1126310
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572927}
[modify] https://crrev.com/f0909821f41ff79e3a7a650cabd94e7d9db24257/content/test/gpu/gpu_tests/trace_integration_test.py

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 6

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/60e91b2439040f1ccd52dfdafe5bcb7829d966ff

commit 60e91b2439040f1ccd52dfdafe5bcb7829d966ff
Author: Juan Antonio Navarro Perez <perezju@chromium.org>
Date: Fri Jul 06 10:46:57 2018

[Telemetry] Remove unused model.IsSliceOrAsyncSlice

Bug:  chromium:860297 
Change-Id: I0f0c65494cdd71765278b8b15c55fc786e921187
Reviewed-on: https://chromium-review.googlesource.com/1127678
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>

[modify] https://crrev.com/60e91b2439040f1ccd52dfdafe5bcb7829d966ff/telemetry/telemetry/timeline/model.py

Project Member

Comment 10 by bugdroid1@chromium.org, Jul 6

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

commit 0b4982997c353a84ea9735374540a1c27976dcc2
Author: Juan Antonio Navarro Perez <perezju@chromium.org>
Date: Fri Jul 06 11:29:41 2018

[tools/perf] Switch clients to model.GetFirstRenderer methods

The methods:

  model.GetRenderer{Thread,Process}FromTabId(..)

are now replaced by:

  model.GetFirstRenderer{Thread,Process}(..)

Depends on CL:
https://chromium-review.googlesource.com/c/catapult/+/1126921

Bug:  860297 
Change-Id: Iec8fbc22e5d7c0e2fbe29e3656e5d9d6f184045f
Reviewed-on: https://chromium-review.googlesource.com/1126936
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572942}
[modify] https://crrev.com/0b4982997c353a84ea9735374540a1c27976dcc2/tools/perf/benchmarks/blink_perf.py
[modify] https://crrev.com/0b4982997c353a84ea9735374540a1c27976dcc2/tools/perf/measurements/rendering.py
[modify] https://crrev.com/0b4982997c353a84ea9735374540a1c27976dcc2/tools/perf/measurements/smoothness.py
[modify] https://crrev.com/0b4982997c353a84ea9735374540a1c27976dcc2/tools/perf/measurements/task_execution_time.py
[modify] https://crrev.com/0b4982997c353a84ea9735374540a1c27976dcc2/tools/perf/measurements/thread_times.py
[modify] https://crrev.com/0b4982997c353a84ea9735374540a1c27976dcc2/tools/perf/measurements/timeline_controller.py
[modify] https://crrev.com/0b4982997c353a84ea9735374540a1c27976dcc2/tools/perf/measurements/v8_gc_times.py

Project Member

Comment 11 by bugdroid1@chromium.org, Jul 6

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

commit 9e50c7fe69aeb55744f0c0e4cf4caadaa740838f
Author: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Fri Jul 06 16:42:58 2018

Roll src/third_party/catapult 5715e9860db3..60e91b243904 (1 commits)

https://chromium.googlesource.com/catapult.git/+log/5715e9860db3..60e91b243904


git log 5715e9860db3..60e91b243904 --date=short --no-merges --format='%ad %ae %s'
2018-07-06 perezju@chromium.org [Telemetry] Remove unused model.IsSliceOrAsyncSlice


Created with:
  gclient setdep -r src/third_party/catapult@60e91b243904

The AutoRoll server is located here: https://catapult-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG= chromium:860297 
TBR=sullivan@chromium.org

Change-Id: I956d5d2cc56dd3f9c359d2fb8d5e0e53dfe14f16
Reviewed-on: https://chromium-review.googlesource.com/1127819
Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#572992}
[modify] https://crrev.com/9e50c7fe69aeb55744f0c0e4cf4caadaa740838f/DEPS

Project Member

Comment 12 by bugdroid1@chromium.org, Jul 9

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/d8e6a8203fc27d72ca59b76b68d962aaf46a614c

commit d8e6a8203fc27d72ca59b76b68d962aaf46a614c
Author: Juan Antonio Navarro Perez <perezju@chromium.org>
Date: Mon Jul 09 08:51:00 2018

[Telemetry] Remove GetRendererThreadFromTabId

Deprecated methods no longer used. Also remove all mechanisms within
Telemetry to include and import TAB_ID_PART from Telemetry traces.

Depends on CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1126936

Bug:  chromium:860297 
Bug:  chromium:499207 
Change-Id: Id2615b9fa69cf8b76c540284a576a823bbe6affd
Reviewed-on: https://chromium-review.googlesource.com/1127675
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>
Reviewed-by: Ned Nguyen <nednguyen@google.com>

[modify] https://crrev.com/d8e6a8203fc27d72ca59b76b68d962aaf46a614c/tracing/tracing/trace_data/trace_data.py
[delete] https://crrev.com/9eaedb7fa00f7b1c3bba87eeb82f75e7a147f8d2/telemetry/telemetry/timeline/tab_id_importer_unittest.py
[modify] https://crrev.com/d8e6a8203fc27d72ca59b76b68d962aaf46a614c/telemetry/telemetry/timeline/model.py
[delete] https://crrev.com/9eaedb7fa00f7b1c3bba87eeb82f75e7a147f8d2/telemetry/telemetry/timeline/tab_id_importer.py
[modify] https://crrev.com/d8e6a8203fc27d72ca59b76b68d962aaf46a614c/tracing/tracing/trace_data/trace_data_unittest.py
[modify] https://crrev.com/d8e6a8203fc27d72ca59b76b68d962aaf46a614c/telemetry/telemetry/value/trace_unittest.py
[modify] https://crrev.com/d8e6a8203fc27d72ca59b76b68d962aaf46a614c/telemetry/telemetry/internal/browser/tab_unittest.py
[modify] https://crrev.com/d8e6a8203fc27d72ca59b76b68d962aaf46a614c/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py

Components: Speed>Telemetry
Project Member

Comment 14 by bugdroid1@chromium.org, Jul 9

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

commit 21a61205903a06cbc01bd3ba68a9df1b1f0a817b
Author: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Mon Jul 09 11:06:45 2018

Roll src/third_party/catapult 9eaedb7fa00f..d8e6a8203fc2 (1 commits)

https://chromium.googlesource.com/catapult.git/+log/9eaedb7fa00f..d8e6a8203fc2


git log 9eaedb7fa00f..d8e6a8203fc2 --date=short --no-merges --format='%ad %ae %s'
2018-07-09 perezju@chromium.org [Telemetry] Remove GetRendererThreadFromTabId


Created with:
  gclient setdep -r src/third_party/catapult@d8e6a8203fc2

The AutoRoll server is located here: https://catapult-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG= chromium:860297 , chromium:499207 
TBR=sullivan@chromium.org

Change-Id: I36e123df65c020a6656e75b86162652790fcbf6f
Reviewed-on: https://chromium-review.googlesource.com/1128799
Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#573267}
[modify] https://crrev.com/21a61205903a06cbc01bd3ba68a9df1b1f0a817b/DEPS

Status: Fixed (was: Assigned)

Comment 16 by benhenry@google.com, Jan 16 (6 days ago)

Components: Test>Telemetry

Comment 17 by benhenry@google.com, Jan 16 (6 days ago)

Components: -Speed>Telemetry

Sign in to add a comment