Use filtering rules on the perf waterfall for ad-tagging |
||||||
Issue descriptionWe want to evaluate the performance impact of ad-tagging, so we probably want to test this config on the perf waterfall. There were previously some experiments by maxlg and robertogden to get filtering rules working with telemetry. We should productionize them.
,
Apr 27 2018
,
Apr 27 2018
I found a bit of prior art here: https://cs.chromium.org/chromium/src/tools/perf/contrib/vr_benchmarks/BUILD.gn Where it seems like VR telemetry tests manually set up a profile. +Ned, do you think something like that could work for us rather than editing telemetry internals? I'll need to dig in to see exactly how their solution works. For more info, here is what Ned recommended over email: """ I think we probably need to extend the core Telemetry browser option to add a field like ads_filter_ruleset=<file path>. The user data dir is set of Telemetry is set in https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py?rcl=67d01a21077fd59a0a64698c559b6b7491d69f47&l=172 Right here, we can make sure that after user data dir is set, we copy the ads_filter_ruleset to a proper location within user data dir. Once that param of browser_options.BrowerOptions is added, we can then modify the base PerfBenchmark as mentioned. *note: I would just add a new field to BrowserOptions class and avoid increase Telemetry's commandline options to keep the runnable script simple. """
,
Apr 28 2018
Not sure how this get slipped. Here's the experimental code I used for the analysis: https://chromium-review.googlesource.com/c/catapult/+/762456/16/telemetry/telemetry/internal/platform/android_platform_backend.py There were several difficulties I encountered and here is how they were resolved: 1. To avoid updating the subresource filter in flight, and avoid the CL size limit (rule files were too big) in cluster telemetry, we made the updated rule files into tar bar. 2. The binary file failed to apply to catapult. We manually generated the git-dif file and pasted it to the cluster telemetry. Hope it helps!
,
May 1 2018
csharrison@: my impression in the email thread is that you want to enable ads tagging filtering for all perf benchmarks and that would be a permanent setting. That's why I recommended adding the ads_filter_ruleset options. If this is just a one off thing on one benchmark of your own, you can certainly follow VR telemetry's design.
,
May 9 2018
Ah yep you're right ideally this will be an option for all telemetry benchmarks, so ads_filter_ruleset makes sense. However, to create the file, we need some pre-processing so we may need to make telemetry depend on some target that generates the ruleset.
,
May 9 2018
#6: in term of the testing target, you can modify telemetry perf test's BUILD target in https://cs.chromium.org/chromium/src/tools/perf/BUILD.gn?q=tools/perf/BUILD.gn&l=1 For example, we do make all telemetry perf benchmarks depend on field trials through https://cs.chromium.org/chromium/src/tools/perf/BUILD.gn?rcl=e079f1560454dfe7b2a19d10527d963583c48acb&l=55
,
May 11 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d4c4b89c00d8933a1b31692c03e2cf835b79a9f3 commit d4c4b89c00d8933a1b31692c03e2cf835b79a9f3 Author: Charlie Harrison <csharrison@chromium.org> Date: Fri May 11 13:12:50 2018 Add ruleset indexing action as data_dep to telemetry tests This allows telemetry to have access to an indexed ruleset, without having to specify one in the source tree (which would need to be updated every time the format version changes). Follow-ups will copy this into telemetry's user-data-dir. Bug: 823982 Change-Id: Iba551b1cd6137d73bed8dfbb0bc69d00f32be7ab Reviewed-on: https://chromium-review.googlesource.com/1052860 Reviewed-by: Josh Karlin <jkarlin@chromium.org> Reviewed-by: Ned Nguyen <nednguyen@google.com> Commit-Queue: Charlie Harrison <csharrison@chromium.org> Cr-Commit-Position: refs/heads/master@{#557855} [modify] https://crrev.com/d4c4b89c00d8933a1b31692c03e2cf835b79a9f3/components/subresource_filter/tools/BUILD.gn [modify] https://crrev.com/d4c4b89c00d8933a1b31692c03e2cf835b79a9f3/components/subresource_filter/tools/ruleset_converter/BUILD.gn [modify] https://crrev.com/d4c4b89c00d8933a1b31692c03e2cf835b79a9f3/tools/perf/BUILD.gn
,
May 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c6977c0579618f7b884842d6f5906cf5a171ca0d commit c6977c0579618f7b884842d6f5906cf5a171ca0d Author: Boris Sazonov <bsazonov@chromium.org> Date: Mon May 14 14:47:58 2018 Revert "Add ruleset indexing action as data_dep to telemetry tests" This reverts commit d4c4b89c00d8933a1b31692c03e2cf835b79a9f3. Reason for revert: broke official build on Android. Builder link: http://uberchromegw/i/official.android/builders/official-arm/builds/3206 Original change's description: > Add ruleset indexing action as data_dep to telemetry tests > > This allows telemetry to have access to an indexed ruleset, without > having to specify one in the source tree (which would need to be > updated every time the format version changes). > > Follow-ups will copy this into telemetry's user-data-dir. > > Bug: 823982 > Change-Id: Iba551b1cd6137d73bed8dfbb0bc69d00f32be7ab > Reviewed-on: https://chromium-review.googlesource.com/1052860 > Reviewed-by: Josh Karlin <jkarlin@chromium.org> > Reviewed-by: Ned Nguyen <nednguyen@google.com> > Commit-Queue: Charlie Harrison <csharrison@chromium.org> > Cr-Commit-Position: refs/heads/master@{#557855} TBR=jkarlin@chromium.org,nednguyen@google.com,csharrison@chromium.org Bug: 842573, 823982 Change-Id: I954fcbaec79ace212d252864f8d48c115a28cd37 Reviewed-on: https://chromium-review.googlesource.com/1057308 Reviewed-by: Boris Sazonov <bsazonov@chromium.org> Reviewed-by: Charlie Harrison <csharrison@chromium.org> Commit-Queue: Boris Sazonov <bsazonov@chromium.org> Cr-Commit-Position: refs/heads/master@{#558304} [modify] https://crrev.com/c6977c0579618f7b884842d6f5906cf5a171ca0d/components/subresource_filter/tools/BUILD.gn [modify] https://crrev.com/c6977c0579618f7b884842d6f5906cf5a171ca0d/components/subresource_filter/tools/ruleset_converter/BUILD.gn [modify] https://crrev.com/c6977c0579618f7b884842d6f5906cf5a171ca0d/tools/perf/BUILD.gn
,
May 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e5f2cb2433516e75bf31428c1280b8b3b84b4dfc commit e5f2cb2433516e75bf31428c1280b8b3b84b4dfc Author: Boris Sazonov <bsazonov@chromium.org> Date: Mon May 14 17:51:52 2018 Revert "Add ruleset indexing action as data_dep to telemetry tests" This reverts commit d4c4b89c00d8933a1b31692c03e2cf835b79a9f3. Reason for revert: broke official build on Android. Builder link: http://uberchromegw/i/official.android/builders/official-arm/builds/3206 Original change's description: > Add ruleset indexing action as data_dep to telemetry tests > > This allows telemetry to have access to an indexed ruleset, without > having to specify one in the source tree (which would need to be > updated every time the format version changes). > > Follow-ups will copy this into telemetry's user-data-dir. > > Bug: 823982 > Change-Id: Iba551b1cd6137d73bed8dfbb0bc69d00f32be7ab > Reviewed-on: https://chromium-review.googlesource.com/1052860 > Reviewed-by: Josh Karlin <jkarlin@chromium.org> > Reviewed-by: Ned Nguyen <nednguyen@google.com> > Commit-Queue: Charlie Harrison <csharrison@chromium.org> > Cr-Commit-Position: refs/heads/master@{#557855} TBR=jkarlin@chromium.org,nednguyen@google.com,csharrison@chromium.org Bug: 842573, 823982 Change-Id: I954fcbaec79ace212d252864f8d48c115a28cd37 Reviewed-on: https://chromium-review.googlesource.com/1057308 Reviewed-by: Boris Sazonov <bsazonov@chromium.org> Reviewed-by: Charlie Harrison <csharrison@chromium.org> Commit-Queue: Boris Sazonov <bsazonov@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#558304}(cherry picked from commit c6977c0579618f7b884842d6f5906cf5a171ca0d) Reviewed-on: https://chromium-review.googlesource.com/1057711 Cr-Commit-Position: refs/branch-heads/3430@{#3} Cr-Branched-From: e7023f8c2d97b4e7c0a109659dfb7e3ed4961ae3-refs/heads/master@{#558173} [modify] https://crrev.com/e5f2cb2433516e75bf31428c1280b8b3b84b4dfc/components/subresource_filter/tools/BUILD.gn [modify] https://crrev.com/e5f2cb2433516e75bf31428c1280b8b3b84b4dfc/components/subresource_filter/tools/ruleset_converter/BUILD.gn [modify] https://crrev.com/e5f2cb2433516e75bf31428c1280b8b3b84b4dfc/tools/perf/BUILD.gn
,
May 15 2018
The following revision refers to this bug: https://chrome-internal.googlesource.com/clank/internal/apps/+/39054e6ae9759a49e2ccaf215de5b25b7cb3f854 commit 39054e6ae9759a49e2ccaf215de5b25b7cb3f854 Author: Charlie Harrison <csharrison@google.com> Date: Tue May 15 13:07:38 2018
,
May 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/catapult/+/35cefb8d8a2fde1e2506a3688d6fce3825387fea commit 35cefb8d8a2fde1e2506a3688d6fce3825387fea Author: Charlie Harrison <csharrison@chromium.org> Date: Tue May 15 13:20:28 2018 Introduce a BrowserOption for seeding a default profile This will be used for ad-tagging work, which needs two pieces: 1. Copying a generated file into <user-data-dir>. This file is a ruleset which Chrome uses to compute whether resources match ad requests. 2. Seeding some JSON prefs. We need to have the initial Local State prefs seeded with the correct ruleset version, otherwise it won't check the for the file that we copied in (1). Both (1) and (2) can be satisfied using an API that can copy over exiting files to the profile, when it is created. This CL adds a new BrowserOption, |profile_files_to_copy|, which takes a list of tuples (source_path, dest_path), where dest_path is relative to the created profile, and copies files in source_path to dest_path on creation. This CL adds support for this option in PossibleDesktopBrowser. Follow-ups will add Android support. See crrev.com/c/1054590 for the Chromium CL which uses this new API. This CL also fixes a small bug when setting browser_options.profile_dir in PossibleDesktopBrowser. Bug: chromium:823982 Change-Id: I3ea8c4f3231fa145c0cf278afc2d16995b73746d Reviewed-on: https://chromium-review.googlesource.com/1054519 Commit-Queue: Charlie Harrison <csharrison@chromium.org> Reviewed-by: Ned Nguyen <nednguyen@google.com> [modify] https://crrev.com/35cefb8d8a2fde1e2506a3688d6fce3825387fea/telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py [modify] https://crrev.com/35cefb8d8a2fde1e2506a3688d6fce3825387fea/telemetry/telemetry/internal/browser/browser_options.py [add] https://crrev.com/35cefb8d8a2fde1e2506a3688d6fce3825387fea/telemetry/telemetry/internal/backends/chrome/possible_desktop_browser_unittest.py
,
May 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/08b24cf934a9a73b384c36be145d74ec7bab465a commit 08b24cf934a9a73b384c36be145d74ec7bab465a Author: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Date: Tue May 15 14:54:39 2018 Roll src/third_party/catapult/ 075ca3000..35cefb8d8 (1 commit) https://chromium.googlesource.com/catapult.git/+log/075ca3000abf..35cefb8d8a2f $ git log 075ca3000..35cefb8d8 --date=short --no-merges --format='%ad %ae %s' 2018-05-15 csharrison Introduce a BrowserOption for seeding a default profile Created with: roll-dep src/third_party/catapult BUG= chromium:823982 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. TBR=sullivan@chromium.org Change-Id: Ibe75061d90cddd4ed6e48831254b094521feaa76 Reviewed-on: https://chromium-review.googlesource.com/1059695 Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Cr-Commit-Position: refs/heads/master@{#558703} [modify] https://crrev.com/08b24cf934a9a73b384c36be145d74ec7bab465a/DEPS
,
May 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f40e03bc161679d4e46902ea0b09519de533fac9 commit f40e03bc161679d4e46902ea0b09519de533fac9 Author: Charlie Harrison <csharrison@chromium.org> Date: Tue May 15 19:37:21 2018 Reland: Add ruleset indexing action as data_dep to telemetry tests Previously landed in crrev.com/c/1052860 and reverted in crrev.com/c/1057308. This should be safe to land, due to the internal change http://crrev.com/i/625646, which stops official Android builds from setting enable_resource_whitelist_generation in args.gn. Original description: This allows telemetry to have access to an indexed ruleset, without having to specify one in the source tree (which would need to be updated every time the format version changes). Follow-ups will copy this into telemetry's user-data-dir. Bug: 823982 Change-Id: I8c9208f1148820340d1dc587e28fb0dfbaa354ee Reviewed-on: https://chromium-review.googlesource.com/1057422 Reviewed-by: Ned Nguyen <nednguyen@google.com> Commit-Queue: Charlie Harrison <csharrison@chromium.org> Cr-Commit-Position: refs/heads/master@{#558809} [modify] https://crrev.com/f40e03bc161679d4e46902ea0b09519de533fac9/components/subresource_filter/tools/BUILD.gn [modify] https://crrev.com/f40e03bc161679d4e46902ea0b09519de533fac9/components/subresource_filter/tools/ruleset_converter/BUILD.gn [modify] https://crrev.com/f40e03bc161679d4e46902ea0b09519de533fac9/tools/perf/BUILD.gn
,
May 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c6680fba5c49a443e2bdeb847eda3815d8b373e0 commit c6680fba5c49a443e2bdeb847eda3815d8b373e0 Author: Charlie Harrison <csharrison@chromium.org> Date: Thu May 17 19:14:01 2018 Setup perf benchmarks to use ad tagging/filtering ruleset This CL depends on the catapult CL here: https://chromium-review.googlesource.com/c/catapult/+/1054519 It uses the new APIs to seed the profile with the ad tagging ruleset, along with a prefs file that specifies which version of the ruleset is available. Bug: 823982 Change-Id: Ib29f229a30c7ad181701c7acb24b5127eccbcff5 Reviewed-on: https://chromium-review.googlesource.com/1054590 Commit-Queue: Charlie Harrison <csharrison@chromium.org> Reviewed-by: Ned Nguyen <nednguyen@google.com> Cr-Commit-Position: refs/heads/master@{#559634} [add] https://crrev.com/c6680fba5c49a443e2bdeb847eda3815d8b373e0/tools/perf/core/default_local_state.json [modify] https://crrev.com/c6680fba5c49a443e2bdeb847eda3815d8b373e0/tools/perf/core/perf_benchmark.py [add] https://crrev.com/c6680fba5c49a443e2bdeb847eda3815d8b373e0/tools/perf/core/perf_benchmark_unittest.py
,
May 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/20917d71479d5412c846c554c6b4da37196acd88 commit 20917d71479d5412c846c554c6b4da37196acd88 Author: Charlie Harrison <csharrison@chromium.org> Date: Fri May 18 14:45:07 2018 Add presubmit to ensure ruleset version for perf benchmarks is up to date This will ensure that updates to the ruleset format version are propagated to tools/perf/core/default_local_state.json. An alternative would be to add a compile step which generates this file, but since it is updated so infrequently, a presubmit seems like a fine solution. Note: This presubmit does _not_ guard against the case when default_local_state.json changes but the indexed ruleset version does not. That should really never happen though. Bug: 823982 Change-Id: I3c64047a66a24e8cceb61b8acd45a5bce9168e0d Reviewed-on: https://chromium-review.googlesource.com/1064507 Reviewed-by: Josh Karlin <jkarlin@chromium.org> Commit-Queue: Charlie Harrison <csharrison@chromium.org> Cr-Commit-Position: refs/heads/master@{#559889} [modify] https://crrev.com/20917d71479d5412c846c554c6b4da37196acd88/components/subresource_filter/core/common/PRESUBMIT.py
,
May 22 2018
Charles: is this work done?
,
May 22 2018
It is finished for desktop. There are two CLs left for Android. Fixing chowning in catapul/devil: https://chromium-review.googlesource.com/c/catapult/+/1066361 Supporting profile_files_to_copy in PossibleAndroidBrowser: https://chromium-review.googlesource.com/c/catapult/+/1066480
,
Jun 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/catapult/+/b520f6298c3e149100f869b57b6c19c1f8c0df60 commit b520f6298c3e149100f869b57b6c19c1f8c0df60 Author: Charlie Harrison <csharrison@chromium.org> Date: Thu Jun 14 21:45:08 2018 Add support for profile_files_to_copy in PossibleAndroidBrowser This CL makes one other behavior change: in PushProfile we push files to the sdcard staging area with delete_device_stale=True. This means that the sdcard will not accumulate junk from previous test iterations. IMO this behavior is more consistent with what PushProfile should do. Bug: chromium:823982 Change-Id: I279bf625fb233ad263d500605d940ace0639751a Reviewed-on: https://chromium-review.googlesource.com/1066480 Commit-Queue: Ned Nguyen <nednguyen@google.com> Reviewed-by: Ned Nguyen <nednguyen@google.com> Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org> [modify] https://crrev.com/b520f6298c3e149100f869b57b6c19c1f8c0df60/telemetry/telemetry/internal/backends/chrome/android_browser_finder_unittest.py [add] https://crrev.com/b520f6298c3e149100f869b57b6c19c1f8c0df60/common/py_utils/py_utils/file_util.py [modify] https://crrev.com/b520f6298c3e149100f869b57b6c19c1f8c0df60/telemetry/telemetry/internal/backends/chrome/android_browser_finder.py [add] https://crrev.com/b520f6298c3e149100f869b57b6c19c1f8c0df60/common/py_utils/py_utils/file_util_unittest.py [modify] https://crrev.com/b520f6298c3e149100f869b57b6c19c1f8c0df60/telemetry/telemetry/internal/platform/android_platform_backend.py [modify] https://crrev.com/b520f6298c3e149100f869b57b6c19c1f8c0df60/telemetry/telemetry/internal/backends/chrome/desktop_browser_finder.py
,
Jun 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/414404c3dae4ab7e63ca571e0491935c41d78aa7 commit 414404c3dae4ab7e63ca571e0491935c41d78aa7 Author: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Date: Fri Jun 15 00:23:16 2018 Roll src/third_party/catapult 42e9609fc92e..b520f6298c3e (3 commits) https://chromium.googlesource.com/catapult.git/+log/42e9609fc92e..b520f6298c3e git log 42e9609fc92e..b520f6298c3e --date=short --no-merges --format='%ad %ae %s' 2018-06-14 csharrison@chromium.org Add support for profile_files_to_copy in PossibleAndroidBrowser 2018-06-14 sullivan@chromium.org Added apps scripts to mail perf sheriffing info to experimental/ 2018-06-14 benjhayden@chromium.org Support multiple story tags in ChartJsonConverter. Created with: gclient setdep -r src/third_party/catapult@b520f6298c3e 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:823982 TBR=sullivan@chromium.org Change-Id: I57109c7cae486eef0e69974e9cdb99dedb801c78 Reviewed-on: https://chromium-review.googlesource.com/1101719 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@{#567495} [modify] https://crrev.com/414404c3dae4ab7e63ca571e0491935c41d78aa7/DEPS
,
Jun 15 2018
Work should be done on this bug. I am firing up some Android and desktop pinpoint jobs to verify.
,
Jul 9
Confirmed via pinpoint this works on Windows but not on Android. On Android we fail to populate profile_files_to_copy because we can't find the out/ directory associated with browser type "android-chromium". On desktop this seems to work because the pinpoint job's browser_type is in GetBuildDirectories.
,
Jul 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b733c418195d21a79adaf2feb6e186e592d30fc5 commit b733c418195d21a79adaf2feb6e186e592d30fc5 Author: Charlie Harrison <csharrison@chromium.org> Date: Tue Jul 10 05:54:26 2018 Default to Release directories to find ad tagging rulesets If browser_type is not set to something that corresponds with an existing directory (e.g. android-chromium), this just defaults us to look for an existing "Release" directory. This shouldn't break anything even if the directory exists but isn't correct. Bug: 823982 Cq-Include-Trybots: master.tryserver.chromium.perf:obbs_fyi Change-Id: I9657bdc1b1499c936b12de62168a5031a3fb2a01 Reviewed-on: https://chromium-review.googlesource.com/1129826 Commit-Queue: Charlie Harrison <csharrison@chromium.org> Reviewed-by: Ned Nguyen <nednguyen@google.com> Cr-Commit-Position: refs/heads/master@{#573627} [modify] https://crrev.com/b733c418195d21a79adaf2feb6e186e592d30fc5/tools/perf/core/perf_benchmark.py [modify] https://crrev.com/b733c418195d21a79adaf2feb6e186e592d30fc5/tools/perf/core/perf_benchmark_unittest.py
,
Jul 10
Confirmed via pinpoint that filtering rules are being checked for Android after the latest CL, so this should be fixed. Let's re-open if we see issues with landing the fieldtrial config for testing. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by robertogden@chromium.org
, Mar 21 2018