owners check taking 72 seconds in some cases (regression) |
|||
Issue descriptionI noticed while working on a large CL, that the "checking owners" step of presubmit was taking a very long time: To reproduce: try patch in https://codereview.chromium.org/2248873002/ to a chromium checkout, and run 'git cl presubmit --upload'. There should be a complaint about the checking owners step taking a long time -- on a Linux machine, I saw it take 71971 milliseconds. The perf hit in this case seems to be a regression starting with the depot_tools change https://codereview.chromium.org/2148153002/. If I revert that change (which has valueable correctness enhancements) from my depot_tools, it only takes 144 milliseconds. load_data_needed_for() alone takes over 50 seconds; almost all of this time is spent in the _owners_for() function. It seems like there's some quadratic behavior going on in the number of files and rules, which might not be that big a deal, except that fnmatch.fnmatch might be much slower than I'd expect. It doesn't seem like load_data_needed_for really needs to process rules at all; it's just trying to load in the set of potentially relevant OWNERS files. Changing that behavior seems to drop the perf down to the subsecond range. We may be able to further accelerate the fnmatch-based checks via regex conversion (fnmatch.translate), but maybe this is overkill. I'll try to characterize the perf a little more.
,
Aug 31 2016
@nick - let me know if/when you have something you want me to review, or if you want me to take this over. Otherwise I'll assume that you're on it or it isn't a problem.
,
Sep 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools.git/+/7e16cf303221bbcf81d632924e19ddc888da9c3b commit 7e16cf303221bbcf81d632924e19ddc888da9c3b Author: nick <nick@chromium.org> Date: Fri Sep 16 23:05:05 2016 owners.py: partial fix for owners-check perf regression fnmatch.fnmatch seems to fall off a performance cliff once you start cycling through more patterns than can fit in its internal cache. BUG= 642793 Review-Url: https://codereview.chromium.org/2293233002 [modify] https://crrev.com/7e16cf303221bbcf81d632924e19ddc888da9c3b/owners.py
,
Sep 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/build.git/+/1a9a50a459c9e3c081719d3e835cac213c319136 commit 1a9a50a459c9e3c081719d3e835cac213c319136 Author: recipe-roller <recipe-roller@chromium.org> Date: Fri Sep 16 23:14:29 2016 Roll recipe dependencies (trivial). This is an automated CL created by the recipe roller. This CL rolls recipe changes from upstream projects (e.g. depot_tools) into downstream projects (e.g. tools/build). More info is at https://goo.gl/zkKdpD. Use https://goo.gl/noib3a to file a bug (or complain) depot_tools: https://crrev.com/7e16cf303221bbcf81d632924e19ddc888da9c3b owners.py: partial fix for owners-check perf regression (nick@chromium.org) TBR=martiniss@chromium.org,phajdan.jr@chromium.org BUG= 642793 Recipe-Tryjob-Bypass-Reason: Autoroller Bugdroid-Send-Email: False Review-Url: https://codereview.chromium.org/2348993003 [modify] https://crrev.com/1a9a50a459c9e3c081719d3e835cac213c319136/infra/config/recipes.cfg
,
Sep 16 2016
The following revision refers to this bug: https://chrome-internal.googlesource.com/chrome/tools/build_limited/scripts/slave/+/6049d2b981dd78d757c052d393af74257cb198a6 commit 6049d2b981dd78d757c052d393af74257cb198a6 Author: recipe-roller <recipe-roller@chromium.org> Date: Fri Sep 16 23:26:52 2016
,
Sep 16 2016
This should be fixed now. There's still a 2-3x performance opportunity here, but we're back in the subsecond range for a large CL, which is acceptable. It's possible that fixing bug 643390 would significantly restructure the code, and further optimizations (suggestions from earlier patchset of the fix CL) might run afoul of that bug.
,
Sep 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/infra/infra.git/+/7d6811c4c58a9e280bca7bf65a8dfde001bef42a commit 7d6811c4c58a9e280bca7bf65a8dfde001bef42a Author: recipe-roller <recipe-roller@chromium.org> Date: Fri Sep 16 23:37:07 2016 Roll recipe dependencies (trivial). This is an automated CL created by the recipe roller. This CL rolls recipe changes from upstream projects (e.g. depot_tools) into downstream projects (e.g. tools/build). More info is at https://goo.gl/zkKdpD. Use https://goo.gl/noib3a to file a bug (or complain) build: https://crrev.com/ba9de887ec268453298caa8641de3cc91dbfb818 Roll recipe dependencies (trivial). (recipe-roller@chromium.org) https://crrev.com/e1d15cd7795cc1c5465983af8a49669242857107 Add rtc_stats_unittests to the test suite (ehmaldonado@chromium.org) https://crrev.com/37294244d9d54d639a083d382da68c50f8fe14e5 Add linux_chromium_headless_dbg trybot (perezju@chromium.org) https://crrev.com/f0f4c4a27746dee8a53234f03e452dba556deb55 Reland of Enable the ninja up-to-date check for Android builders (agrieve@chromium.org) https://crrev.com/2744734cf3aa1d8a02d0f0b7abf99635e9f2aa48 Revert of Enable the ninja up-to-date check for Android builders (patchset #1 id:1 of https://codereview.chromium.org/2343563003/ ) (agrieve@chromium.org) https://crrev.com/1575589a1d8f61fee45ec53ccbf77f073fd38847 recipe_modules/chromite: Use "build_type". (dnj@chromium.org) https://crrev.com/5799bab975aa3eda6785f36d9f7879549074b28b Reland of Enable the ninja up-to-date check for Android builders (agrieve@chromium.org) https://crrev.com/ecdf065c9d2a6e28d86c8ad333432b497158ee7a chromium.android: Enable swarming on Android arm64 builder (bpastene@chromium.org) https://crrev.com/7a71133c89aa2a770fc2326188306cf6a3a6f1e1 Revert of Enable the ninja up-to-date check for Android builders (patchset #1 id:1 of https://codereview.chromium.org/2343953002/ ) (agrieve@chromium.org) https://crrev.com/c65424c419105e0720cc0de09fb9e7a36c3d4601 Pass ninja -n in the ninja -d explain step to avoid more work (agrieve@chromium.org) https://crrev.com/8091fad9d075ecec116e1740360ff955a7654c4a Remove use_isolate from Marshmallow 64 bit Tester (bpastene@chromium.org) https://crrev.com/2f0a7c064259dc0e0bf375c2d1771544f5647a1c Changed sequence of package_build step to be before package_build_for_bisect (miimnk@google.com) https://crrev.com/194bd0247efb60371dbcae87731f13dce4fa7934 Roll recipe dependencies (trivial). (recipe-roller@chromium.org) https://crrev.com/d2845f7e87aa167cf7df78003b5768f948803d07 Roll recipe dependencies (trivial). (recipe-roller@chromium.org) https://crrev.com/eb686c79229ef3cf3b39e6a0807f4c98e3e93cf9 Add BuildBucket manifest scheduling support. (dnj@chromium.org) https://crrev.com/537cdb699a70d5d55f3eefb923eae1dc5104a8eb Add goma to wasm waterfall (sbc@chromium.org) https://crrev.com/b6d326d6f6f9608bba307109cc4a1c5efc5946bb Run the ninja "up-to-date" check for all compiles, but just as an fyi (agrieve@chromium.org) https://crrev.com/07a33586eff8451bbc84366055c3cfa36e7b6fea Roll recipe dependencies (nontrivial). (recipe-roller@chromium.org) https://crrev.com/79d8d758281609aa49b9b9af7e18cc187c2ee814 Disable CompilerInfoCache for a while (shinyak@chromium.org) https://crrev.com/a8f07009101d46553ea5d4bce1353ecb365dd3a5 Revert "Roll recipe dependencies (nontrivial)." (tandrii@chromium.org) https://crrev.com/cbca79967afa4f28df6b0ede337db6138eeb167b Add asan=1 to GYP_DEFINES for Dart asan builds (whesse@google.com) https://crrev.com/25b922d8edc1927c3af4bb4a8f7db6434096dca0 WebRTC: Disable iOS API Framework Builder. (ehmaldonado@chromium.org) https://crrev.com/769dbabae3b7ff11023dd1b393fcd2a12e165c65 Make sure goma_ctl is running during wasm_llvm build (sbc@chromium.org) https://crrev.com/2e89429885f7c2503565cb12d564d0f6f58a42f1 Fix Webkit capitalization for N CTS tests. (mikecase@chromium.org) https://crrev.com/e549894cc4f86f3f2107e3686d561ff83331a267 crashpad/continuous: Use bot_update. (dnj@chromium.org) https://crrev.com/f74386f7e19ff3f32693d222262d1a3ee79799ef Roll recipe dependencies (trivial). (recipe-roller@chromium.org) https://crrev.com/1a9a50a459c9e3c081719d3e835cac213c319136 Roll recipe dependencies (trivial). (recipe-roller@chromium.org) depot_tools: https://crrev.com/f46c20fcee6e6a0a7d75788847632cd4ac18e2e9 codereview.settings: add GIT_NUMBER_FOOTER setting. (tandrii@chromium.org) https://crrev.com/5d0a0421ce27046c94177511a05699316ec8097a git_cl: update outdated TODOs. (tandrii@chromium.org) https://crrev.com/73449b0bd49eab1e152f419102123d734896da98 Gerrit git cl land: abort if not uploaded. (tandrii@chromium.org) https://crrev.com/bf42940536f6c0c123a9e6278c20bc38729b3717 git cl land to refs/pending: remove unused arg. (tandrii@chromium.org) https://crrev.com/7475196d4c32d66e1c199bf24945b7ae28255e13 repo: update to v1.12.17-cr1 (vapier@chromium.org) https://crrev.com/adcd4b78d5f35da535ccf82a221afb9fa389f631 presubmit_support: Remove a noisy logging.debug() (thakis@chromium.org) https://crrev.com/18ca30ca804679ee624a52e73017d234a8c0008f Teach bot_update to remove partially deleted git repos. (vadimsh@chromium.org) https://crrev.com/972ac5040176acd90c8a1ce412f75d19f77cc4e8 bot_update: ensure correct depot_tools checkout is used. (tandrii@chromium.org) https://crrev.com/15a248123d9032061486cd2d4b3f64369c93a9a5 Revert of bot_update: ensure correct depot_tools checkout is used. (patchset #2 id:20001 of https://codereview.chromium.org/2346973003/ ) (tandrii@chromium.org) https://crrev.com/7f245d07b2282f9847072fccddf7162a7e632a2d Bump git-on-windows bleeding edge version to 2.10.0. (vadimsh@chromium.org) https://crrev.com/6ac12ffd596e338c43e25dc3889e8ac552c2e885 Make bot_update.py print git version it uses. (vadimsh@chromium.org) https://crrev.com/7e16cf303221bbcf81d632924e19ddc888da9c3b owners.py: partial fix for owners-check perf regression (nick@chromium.org) TBR=martiniss@chromium.org,phajdan.jr@chromium.org BUG=646165,none,646838,632008,webrtc:6372,chromium:647812,642493,647446,chromium:627996,642793,635641,645662,chromium:632203,647046,609225,642759 Recipe-Tryjob-Bypass-Reason: Autoroller Bugdroid-Send-Email: False Review-Url: https://codereview.chromium.org/2345413002 [modify] https://crrev.com/7d6811c4c58a9e280bca7bf65a8dfde001bef42a/infra/config/recipes.cfg
,
Sep 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/603782693c407e3a7350d7f65846625d07aaf217 commit 603782693c407e3a7350d7f65846625d07aaf217 Author: recipe-roller <recipe-roller@chromium.org> Date: Fri Sep 16 23:35:10 2016 Roll recipe dependencies (trivial). This is an automated CL created by the recipe roller. This CL rolls recipe changes from upstream projects (e.g. depot_tools) into downstream projects (e.g. tools/build). More info is at https://goo.gl/zkKdpD. Use https://goo.gl/noib3a to file a bug (or complain) build: https://crrev.com/1a9a50a459c9e3c081719d3e835cac213c319136 Roll recipe dependencies (trivial). (recipe-roller@chromium.org) depot_tools: https://crrev.com/7e16cf303221bbcf81d632924e19ddc888da9c3b owners.py: partial fix for owners-check perf regression (nick@chromium.org) TBR=martiniss@chromium.org,phajdan.jr@chromium.org BUG= 642793 Recipe-Tryjob-Bypass-Reason: Autoroller Bugdroid-Send-Email: False Review-Url: https://codereview.chromium.org/2349733004 Cr-Commit-Position: refs/heads/master@{#419326} [modify] https://crrev.com/603782693c407e3a7350d7f65846625d07aaf217/infra/config/recipes.cfg |
|||
►
Sign in to add a comment |
|||
Comment 1 by nick@chromium.org
, Aug 31 2016