New issue
Advanced search Search tips

Issue 788035 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

check_network_annotations is too slow on the trybots

Project Member Reported by dpranke@chromium.org, Nov 23 2017

Issue description

It looks like the "check_network_annotations" step takes ~3:30 on "Linux Builder", which isn't great but is probably acceptable. However, it also takes almost 20 minutes on linux_chromium_rel_ng, and that's way too slow (presumably windows would be even worse).

We need to figure out how to make this check run in a couple of minutes, tops, before we can run it in the CQ. I'm disabling it in the meantime.

I apologize for not noticing this earlier :(.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 23 2017

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

commit b7ee89f8ec4e03f8161d0936a07f37d3b3cb2ae3
Author: Dirk Pranke <dpranke@chromium.org>
Date: Thu Nov 23 02:46:52 2017

Disable check_network_annotations on the CQ bots.

The newly-enabled `check_network_annotations` script appears to
take up to 20 minutes to run on linux_chromium_rel_ng; even though
this is run locally on the bot in parallel with tests run under
swarming, this is still much too slow.

We need to figure out how to get this to complete in a couple
minutes, tops, if we want it to run in the CQ.

TBR=rhalavati@chromium.org, msramek@chromium.org
BUG= 788035 

Change-Id: I42b2b07f5d856d89013e372b93d8cce830855806
Reviewed-on: https://chromium-review.googlesource.com/786780
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518829}
[modify] https://crrev.com/b7ee89f8ec4e03f8161d0936a07f37d3b3cb2ae3/tools/traffic_annotation/scripts/check_annotations.py

Status: Started (was: Untriaged)
The test runs in 40s on a z840 machine with 48 cores, 64GB RAM, and SSD harddrive.
Given that the trybot virtual machines are not as capable, maybe I need to move it to swarming bots.
Unfortunately, given that you need to scan source code, I don't think we can easily move it to swarming. At the very least, we'd have to be able to identify a subset of the tree to scan.
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 11 2017

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

commit dffd5dc48c06a528e891868f3af7ef91d9950b94
Author: rhalavati@google.com <rhalavati@google.com>
Date: Mon Dec 11 15:39:23 2017

Extend annotations.xml.

Extends annotations.xml to include incomplete annotations, file paths,
and the list of fields in the incomplete annotations.

Bug:  656607 
Bug:  788035 
Change-Id: Ibabbbd91fa8d618571d3fe00dc32fd0aec0277ed
Reviewed-on: https://chromium-review.googlesource.com/817749
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523102}
[modify] https://crrev.com/dffd5dc48c06a528e891868f3af7ef91d9950b94/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/dffd5dc48c06a528e891868f3af7ef91d9950b94/tools/traffic_annotation/auditor/instance.cc
[modify] https://crrev.com/dffd5dc48c06a528e891868f3af7ef91d9950b94/tools/traffic_annotation/auditor/instance.h
[modify] https://crrev.com/dffd5dc48c06a528e891868f3af7ef91d9950b94/tools/traffic_annotation/auditor/traffic_annotation_auditor.cc
[modify] https://crrev.com/dffd5dc48c06a528e891868f3af7ef91d9950b94/tools/traffic_annotation/auditor/traffic_annotation_auditor.h
[modify] https://crrev.com/dffd5dc48c06a528e891868f3af7ef91d9950b94/tools/traffic_annotation/auditor/traffic_annotation_auditor_unittest.cc
[modify] https://crrev.com/dffd5dc48c06a528e891868f3af7ef91d9950b94/tools/traffic_annotation/auditor/traffic_annotation_exporter.cc
[modify] https://crrev.com/dffd5dc48c06a528e891868f3af7ef91d9950b94/tools/traffic_annotation/auditor/traffic_annotation_exporter.h
[modify] https://crrev.com/dffd5dc48c06a528e891868f3af7ef91d9950b94/tools/traffic_annotation/summary/annotations.xml

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 13 2017

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

commit 437ba3c4c4987abb77b5907e3bce97642ba917ab
Author: rhalavati@google.com <rhalavati@google.com>
Date: Wed Dec 13 13:27:25 2017

Run Traffic Annotation tests on Trybot only on modified files.

Traffic annotations test script on trybot is updated to only consider
the list of changed files.

Bug:  788035 
Bug:  656607 
Change-Id: Ib02f971edc65076c44f4aecf0589b30d381e00f7
Reviewed-on: https://chromium-review.googlesource.com/813635
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523761}
[modify] https://crrev.com/437ba3c4c4987abb77b5907e3bce97642ba917ab/tools/traffic_annotation/scripts/check_annotations.py

Project Member

Comment 7 by bugdroid1@chromium.org, Jan 8 2018

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

commit 36e99a557191d54179c546cc9fdcd323508a2365
Author: rhalavati@google.com <rhalavati@google.com>
Date: Mon Jan 08 09:15:08 2018

Activate Network traffic annotation tests for Linux trybot.

Traffic annotation tests are updated to run on the changed files instead
of the whole repository, which should make tests much faster.

Bug:  690323 
Bug:  788035 
Change-Id: Ie67c1abe02d2f1670e1d5c0b4491682157f390c1
Reviewed-on: https://chromium-review.googlesource.com/842403
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527588}
[modify] https://crrev.com/36e99a557191d54179c546cc9fdcd323508a2365/tools/traffic_annotation/README.md
[modify] https://crrev.com/36e99a557191d54179c546cc9fdcd323508a2365/tools/traffic_annotation/bin/linux64/traffic_annotation_auditor.sha1
[modify] https://crrev.com/36e99a557191d54179c546cc9fdcd323508a2365/tools/traffic_annotation/scripts/check_annotations.py

Project Member

Comment 8 by bugdroid1@chromium.org, Jan 8 2018

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

commit 96de83c487be35a0ca3d55be03198a6bebc4a7f7
Author: John Abd-El-Malek <jam@chromium.org>
Date: Mon Jan 08 15:30:21 2018

Revert "Activate Network traffic annotation tests for Linux trybot."

This reverts commit 36e99a557191d54179c546cc9fdcd323508a2365.

Lots of failures on CQ like this:
Running ['/usr/bin/python', '/b/c/b/linux/src/tools/traffic_annotation/scripts/check_annotations.py', '--build-path', u'/b/c/b/linux/src/out/Release'] in None (env: None)
[Errors]
  (1)	'tools/traffic_annotation/summary/annotations.xml' requires update. It is recommended to run traffic_annotation_auditor locally to do the updates automatically (please refer to tools/traffic_annotation/auditor/README.md), but you can also apply the following edit(s) to do it manually:
	Update line: ' <item id="download_web_contents_frame" hash_code="56351037" type="0" content_hash_code="3657889" os_list="linux,windows" file_path="content/browser/web_contents/web_contents_impl.cc"/>' --> ' <item id="download_web_contents_frame" hash_code="56351037" type="0" content_hash_code="3657889" os_list="windows" file_path="content/browser/web_contents/web_contents_impl.cc"/>'
 If you are using build flags that modify files (like jumbo), rerun the auditor using --all-files switch.
Command ['/usr/bin/python', '/b/c/b/linux/src/tools/traffic_annotation/scripts/check_annotations.py', '--build-path', u'/b/c/b/linux/src/out/Release'] returned exit code 1
step returned non-zero exit code: 1

Original change's description:
> Activate Network traffic annotation tests for Linux trybot.
> 
> Traffic annotation tests are updated to run on the changed files instead
> of the whole repository, which should make tests much faster.
> 
> Bug:  690323 
> Bug:  788035 
> Change-Id: Ie67c1abe02d2f1670e1d5c0b4491682157f390c1
> Reviewed-on: https://chromium-review.googlesource.com/842403
> Reviewed-by: Dirk Pranke <dpranke@chromium.org>
> Reviewed-by: Martin Šrámek <msramek@chromium.org>
> Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#527588}

TBR=dpranke@chromium.org,msramek@chromium.org,rhalavati@chromium.org

Change-Id: If2a8b71463dcfd4d298a125925d64877a78e6c86
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  690323 ,  788035 
Reviewed-on: https://chromium-review.googlesource.com/854073
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527637}
[modify] https://crrev.com/96de83c487be35a0ca3d55be03198a6bebc4a7f7/tools/traffic_annotation/README.md
[modify] https://crrev.com/96de83c487be35a0ca3d55be03198a6bebc4a7f7/tools/traffic_annotation/bin/linux64/traffic_annotation_auditor.sha1
[modify] https://crrev.com/96de83c487be35a0ca3d55be03198a6bebc4a7f7/tools/traffic_annotation/scripts/check_annotations.py

Comment 9 by jam@chromium.org, Jan 8 2018

I've reverted this due to the failures seen.

Given that this has caused 2 problems on the CQ each time it was enabled, please monitor the trybots closely next time when renabling it.

Comment 10 by jam@chromium.org, Jan 8 2018

Cc: jam@chromium.org
btw here are some examples:

https://chromium-review.googlesource.com/c/chromium/src/+/846611
gave
https://ci.chromium.org/buildbot/tryserver.chromium.linux/linux_chromium_rel_ng/618971

Running ['/usr/bin/python', '/b/c/b/linux/src/tools/traffic_annotation/scripts/check_annotations.py', '--build-path', u'/b/c/b/linux/src/out/Release'] in None (env: None)
[Errors]
  (1)	'tools/traffic_annotation/summary/annotations.xml' requires update. It is recommended to run traffic_annotation_auditor locally to do the updates automatically (please refer to tools/traffic_annotation/auditor/README.md), but you can also apply the following edit(s) to do it manually:
	Update line: ' <item id="download_web_contents_frame" hash_code="56351037" type="0" content_hash_code="3657889" os_list="linux,windows" file_path="content/browser/web_contents/web_contents_impl.cc"/>' --> ' <item id="download_web_contents_frame" hash_code="56351037" type="0" content_hash_code="3657889" os_list="windows" file_path="content/browser/web_contents/web_contents_impl.cc"/>'
 If you are using build flags that modify files (like jumbo), rerun the auditor using --all-files switch.
Command ['/usr/bin/python', '/b/c/b/linux/src/tools/traffic_annotation/scripts/check_annotations.py', '--build-path', u'/b/c/b/linux/src/out/Release'] returned exit code 1

but this change didn't change that code that makes the request.



Another example: https://chromium-review.googlesource.com/c/chromium/src/+/842863
https://logs.chromium.org/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux%2Flinux_chromium_rel_ng%2F618947%2F%2B%2Frecipes%2Fsteps%2Fcheck_network_annotations__with_patch_%2F0%2Fstdout
  (1)	'tools/traffic_annotation/summary/annotations.xml' requires update. It is recommended to run traffic_annotation_auditor locally to do the updates automatically (please refer to tools/traffic_annotation/auditor/README.md), but you can also apply the following edit(s) to do it manually:
	Update line: ' <item id="navigation_url_loader" hash_code="63171670" type="0" content_hash_code="129352907" os_list="linux,windows" file_path="content/browser/loader/navigation_url_loader_network_service.cc"/>' --> ' <item id="navigation_url_loader" hash_code="63171670" type="0" content_hash_code="129352907" os_list="windows" file_path="content/browser/loader/navigation_url_loader_network_service.cc"/>'
	Update line: ' <item id="resource_dispatcher_host" hash_code="81157007" type="0" content_hash_code="35725167" os_list="linux,windows" file_path="content/browser/loader/resource_dispatcher_host_impl.cc"/>' --> ' <item id="resource_dispatcher_host" hash_code="81157007" type="0" content_hash_code="35725167" os_list="windows" file_path="content/browser/loader/resource_dispatcher_host_impl.cc"/>'


That change also made some changes to files that make requests, but not to the actual methods. I don't know how that hash is calculated, but it seems like changing any part of the file that makes requests requires updating that tool, which seems like it's broken.
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 11 2018

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

commit 86ff9bc4d831eebf5c0f0b0a04214d1b28e50c3f
Author: rhalavati@google.com <rhalavati@google.com>
Date: Thu Jan 11 07:01:55 2018

Activate Network traffic annotation tests for Linux trybot.

Traffic annotation tests are updated to run on the changed files instead
of the whole repository, which should make tests much faster.

Bug:  690323 
Bug:  788035 
Change-Id: I4eb7c173f5d677cdb9944f02e9c15dfa0a374b10
Reviewed-on: https://chromium-review.googlesource.com/856456
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528573}
[modify] https://crrev.com/86ff9bc4d831eebf5c0f0b0a04214d1b28e50c3f/tools/traffic_annotation/README.md
[modify] https://crrev.com/86ff9bc4d831eebf5c0f0b0a04214d1b28e50c3f/tools/traffic_annotation/bin/linux64/traffic_annotation_auditor.sha1
[modify] https://crrev.com/86ff9bc4d831eebf5c0f0b0a04214d1b28e50c3f/tools/traffic_annotation/scripts/check_annotations.py

Project Member

Comment 12 by bugdroid1@chromium.org, Jan 15 2018

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

commit 64c23fad92332d3b445989bbc535ac9d498311b1
Author: rhalavati@google.com <rhalavati@google.com>
Date: Mon Jan 15 09:07:43 2018

Activate network traffic annotation trybot.

After updating the traffic annotation and check script in
crrev.com/c/863483, network traffic annotation trybot is activated.

Bug:  801192 
Bug:  788035 
Change-Id: I3bdc2bab33445cb8042c773ec5cca4017e8b3027
Reviewed-on: https://chromium-review.googlesource.com/863482
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529232}
[modify] https://crrev.com/64c23fad92332d3b445989bbc535ac9d498311b1/tools/traffic_annotation/scripts/check_annotations.py

Status: Fixed (was: Started)
Project Member

Comment 14 by bugdroid1@chromium.org, Jan 30 2018

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

commit 0e0e6d654fcdae192825324c811798a63dff0ae9
Author: John Abd-El-Malek <jam@chromium.org>
Date: Tue Jan 30 03:11:20 2018

Revert "Activate Network traffic annotation tests for Linux trybot."

This reverts commit 86ff9bc4d831eebf5c0f0b0a04214d1b28e50c3f.

Reason for revert:
flaky, i.e. see

https://ci.chromium.org/buildbot/tryserver.chromium.linux/linux_chromium_rel_ng/634904
https://ci.chromium.org/buildbot/tryserver.chromium.linux/linux_chromium_rel_ng/634934
https://ci.chromium.org/buildbot/tryserver.chromium.linux/linux_chromium_rel_ng/634954

which fail with
Returned error text: Traceback (most recent call last):
  File "/b/c/b/linux/src/tools/clang/scripts/run_tool.py", line 315, in <module>
    sys.exit(main())
  File "/b/c/b/linux/src/tools/clang/scripts/run_tool.py", line 281, in main
    f.write(compile_db.GenerateWithNinja(args.p))
  File "/b/c/b/linux/src/tools/clang/pylib/clang/compile_db.py", line 94, in GenerateWithNinja
    GetNinjaPath(), '-C', path, '-t', 'compdb', 'cc', 'cxx', 'objc',
  File "/b/c/b/linux/src/tools/clang/pylib/clang/compile_db.py", line 76, in GetNinjaPath
    os.path.dirname(os.realpath(__file__)), '..', '..', '..', 'third_party',
AttributeError: 'module' object has no attribute 'realpath'




Original change's description:
> Activate Network traffic annotation tests for Linux trybot.
>
> Traffic annotation tests are updated to run on the changed files instead
> of the whole repository, which should make tests much faster.
>
> Bug:  690323 
> Bug:  788035 
> Change-Id: I4eb7c173f5d677cdb9944f02e9c15dfa0a374b10
> Reviewed-on: https://chromium-review.googlesource.com/856456
> Reviewed-by: Martin Šrámek <msramek@chromium.org>
> Reviewed-by: Dirk Pranke <dpranke@chromium.org>
> Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#528573}

TBR=dpranke@chromium.org,msramek@chromium.org,rhalavati@chromium.org


Bug:  690323 ,  788035 
Change-Id: I048be491eb64c221bb4edeab2e0d2aa051b5faa7
Reviewed-on: https://chromium-review.googlesource.com/892383
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532741}
[modify] https://crrev.com/0e0e6d654fcdae192825324c811798a63dff0ae9/tools/traffic_annotation/scripts/check_annotations.py

Have you ever considered implementing these checks as a clang plugin, not as a tool? That way check results could be cached by compiler caching systems and checks would be almost instant on all platforms. It could be a separate argument to clang plugin to enable annotation checks that would be passed only if according gn arg is enabled (to make sure these checks can be disabled to have current behaviour).
That would be a good idea, but it was not done back then as clang plugins where considered much harder to maintain.
Currently the test speed is increased by reducing it to minimal number of files.

Sign in to add a comment