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

Issue 795449 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 26
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

chromite program startup has gotten too long

Project Member Reported by vapier@chromium.org, Dec 15 2017

Issue description

i haven't tracked this down, just filing so it doesn't get lost.  i suspect the issue is module bloat in our core libs.

we use `cros lint` in many places like `repo upload`.  it takes 1.3s just to process --help.  this seems consistent across chromite scripts.  the nop scenario is supposed to be fast.  when you upload a branch, `cros lint` is run on every commit, as do other programs that use chromite (depending on the repo), so that delay multiplies fast.
 

Comment 1 by yhong@chromium.org, Feb 7 2018

Cc: yhong@chromium.org
`cros lint --help` has to import all modules under chromite/cli/cros/ to get all sub-commands so a solution is to speed up the importing those modules.

After doing a series of experiment, I found that there are two main problems:
1. Importing below packages/modules takes long time:
   - infra_libs.ts_mon
   - httplib2
   - oauth2client.gce
2. Many modules have a global variable "site_config = config_lib.GetConfig()".
  But that method will load the config from file and take around 0.2s.

`cros lint --help` takes only 0.4s if above problems are all solved.

To solve problem 1, I think possible solutions are:
1. Find a good python package for lazy importing such as [this one]("https://pypi.python.org/pypi/lazy-import").
2. Write wrappers which do lazy import and in chromite's python module, only import our wrappers.
3. Write tricky ModuleLoader/ModuleFinder and register them to sys.meta_path to return fake modules which do lazy import once someone actually wants to access the constants/functions of the actual python packages.

I personally prefer solution 2, but still need some advices.

To solve problem 2, it looks like we can just replace all "site_config.blahblah" by "config_lib.GetConfig().blahblah" but still need someone confirm that this solution won't break anything.
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/82e2ac9a251e0870583747b95e6804fe2b8d7cee

commit 82e2ac9a251e0870583747b95e6804fe2b8d7cee
Author: Yong Hong <yhong@chromium.org>
Date: Mon Apr 16 16:20:17 2018

lib/portage_util: Remove unused method.

Also because after removing the unused method `EBuild._GetSHA1ForPath`,
this module don't need to import the big module `chromite.lib.gerrit`,
portage_utils module now costs less time to be imported.

BUG= chromium:795449 
TEST=manually test

Change-Id: I314d7aba84666410a6bb1020c47100e2d63257ab
Reviewed-on: https://chromium-review.googlesource.com/905184
Commit-Ready: Yong Hong <yhong@google.com>
Tested-by: Yong Hong <yhong@google.com>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/82e2ac9a251e0870583747b95e6804fe2b8d7cee/lib/portage_util.py

Project Member

Comment 3 by bugdroid1@chromium.org, Apr 16 2018

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

commit 15719eaa28fa0ddd15395962af7e26d7e7975a8c
Author: chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Mon Apr 16 18:22:55 2018

Roll src/third_party/chromite/ d88eaf531..82e2ac9a2 (1 commit)

https://chromium.googlesource.com/chromiumos/chromite.git/+log/d88eaf5315d4..82e2ac9a251e

$ git log d88eaf531..82e2ac9a2 --date=short --no-merges --format='%ad %ae %s'
2018-02-07 yhong lib/portage_util: Remove unused method.

Created with:
  roll-dep src/third_party/chromite
BUG= chromium:795449 


The AutoRoll server is located here: https://chromite-chromium-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=chrome-os-gardeners@chromium.org

Change-Id: I67bdb12c05e5d242da7c0f3e3e8a1ddf1ea21d80
Reviewed-on: https://chromium-review.googlesource.com/1014195
Commit-Queue: Chromite Chromium Autoroll <chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Reviewed-by: Chromite Chromium Autoroll <chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#551044}
[modify] https://crrev.com/15719eaa28fa0ddd15395962af7e26d7e7975a8c/DEPS

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/15719eaa28fa0ddd15395962af7e26d7e7975a8c

commit 15719eaa28fa0ddd15395962af7e26d7e7975a8c
Author: chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Mon Apr 16 18:22:55 2018

Roll src/third_party/chromite/ d88eaf531..82e2ac9a2 (1 commit)

https://chromium.googlesource.com/chromiumos/chromite.git/+log/d88eaf5315d4..82e2ac9a251e

$ git log d88eaf531..82e2ac9a2 --date=short --no-merges --format='%ad %ae %s'
2018-02-07 yhong lib/portage_util: Remove unused method.

Created with:
  roll-dep src/third_party/chromite
BUG= chromium:795449 


The AutoRoll server is located here: https://chromite-chromium-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=chrome-os-gardeners@chromium.org

Change-Id: I67bdb12c05e5d242da7c0f3e3e8a1ddf1ea21d80
Reviewed-on: https://chromium-review.googlesource.com/1014195
Commit-Queue: Chromite Chromium Autoroll <chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Reviewed-by: Chromite Chromium Autoroll <chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#551044}
[modify] https://crrev.com/15719eaa28fa0ddd15395962af7e26d7e7975a8c/DEPS

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/4e29b62239da2b5b557306ea349bf3a1abc36dd4

commit 4e29b62239da2b5b557306ea349bf3a1abc36dd4
Author: Yong Hong <yhong@chromium.org>
Date: Wed Apr 18 16:30:30 2018

cros_sdk: Resolve the tarball URL only if --download is specified.

Resolving the tarball URL is slow, so we should do it only if we need
the url.

BUG= chromium:795449  of sorts
TEST=manually test `cros_sdk` command.

Change-Id: I686f3ebdb9b0c448ba57ad109116986f8b4e9e82
Reviewed-on: https://chromium-review.googlesource.com/903802
Commit-Ready: Yong Hong <yhong@google.com>
Tested-by: Yong Hong <yhong@google.com>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/4e29b62239da2b5b557306ea349bf3a1abc36dd4/scripts/cros_sdk.py

Project Member

Comment 6 by bugdroid1@chromium.org, Apr 18 2018

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

commit dbf37d75afe3704cb4dba8e623d139392815a253
Author: chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Wed Apr 18 18:17:13 2018

Roll src/third_party/chromite/ 84ba91795..4e29b6223 (1 commit)

https://chromium.googlesource.com/chromiumos/chromite.git/+log/84ba9179592f..4e29b62239da

$ git log 84ba91795..4e29b6223 --date=short --no-merges --format='%ad %ae %s'
2018-02-05 yhong cros_sdk: Resolve the tarball URL only if --download is specified.

Created with:
  roll-dep src/third_party/chromite
BUG= chromium:795449  of sorts


The AutoRoll server is located here: https://chromite-chromium-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=chrome-os-gardeners@chromium.org

Change-Id: Id1c7addda62e3e83c01222aa4d29ac679fffa851
Reviewed-on: https://chromium-review.googlesource.com/1017222
Reviewed-by: Chromite Chromium Autoroll <chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: Chromite Chromium Autoroll <chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#551748}
[modify] https://crrev.com/dbf37d75afe3704cb4dba8e623d139392815a253/DEPS

Comment 7 by la...@chromium.org, Jun 11 2018

Owner: la...@chromium.org
Stealing for saklein@ starter bug.

Comment 8 by la...@chromium.org, Jun 18 2018

Owner: saklein@chromium.org
The most significant time is still coming from the first instance of `site_config = config_lib.GetConfig()`; `cros lint -h` takes ~1s, 0.6s of that is from the config_lib call. The major problem is simply the size of the config being loaded.

The completed site_config has a little more than 5200 entries. It takes a little more than 0.1s just to do the json.loads on the file contents, i.e. after the file has already been read. The file is 4.9MB and has nearly 83k lines. The rest of the time is eaten up by building out those config objects from the raw values. Several years ago Don put in a deepcopy method on the most significant class instead of using the default copy.deepcopy behavior. I was able to add a simple __copy__ method to a number of other config classes to prevent it from using the default copy.copy behavior. Don's change would have made a huge difference due to the volume of deepcopy calls, but the impact of the __copy__ addition is very minor due to the relatively low volume and complexity of those objects.

Another somewhat minor point I'm going to try to look at is the use of nested json encodes. We have nested objects that are json encoding their data dict, which is then being put in a list and again json encoded. This seems entirely unnecessary, and requires >6700 extra json decodes to decode all of those lines.

I'm going to look into the generation process for the config_dump.json file, but an obvious starting point would be to break it out into multiple files and have it only load in the files as needed. Using something like the first letter of the top level config name is really simple, but is not a balanced distribution and does not reflect actual usage patterns. Using the first letter of a hash would be a more even distribution but again doesn't reflect actual usage patterns and would make manually looking up information in those files more difficult. We could potentially go with a full hash, solving the distribution problem, but that would introduce thousands of files and if normal usage patterns access a sufficient number of them, then the repeated FS read overhead could make it a wash or worse.

I don't have any real idea how these configs are used beyond how I've seen them used for local testing, so I would be interested in learning how they're generally used and if there is a good way to break them into groups based on their usage. I suspect that this is difficult to do efficiently with only the top level index, though. Also, obviously, if there's any reason that having them broken out into separate files is currently infeasible or impractical, and what those challenges might be.
I have done some experiments breaking out the configs into files based on the first letter of the config name. It is currently keeping the same main config_dump.json file have the same top level keys but putting in a placeholder value for everything besides the defaults, templates and param sections. It checks for the placeholder value and performs the same logic it did before if it's anything but the placeholder value. The SiteConfig class getitem also looks for the placeholder value and loads in files as they're needed. End result is the old config file works, the new config files work when needed, and we would be able to always load in a subset of them by having those in the config_dump.json file if we wanted to.

`cros lint -h` is about 60% faster - 1.08s down to 0.41s

Use cases that need all of the keys pay about a 0.07s overhead for additional reads, json decodes, etc. - nothing major if we don't do that too often.

There's also a small additional cost of storing and checking the values on every __getitem__ call, and I had to override the iteritems and itervalues methods. Those methods are also almost certainly slower than their builtin counterparts, if nothing else because they also have the __getitem__ overhead for every entry. I plan to characterize these additional costs tomorrow, as well as verify all required methods have been overridden (values and items at least still need overrides).

Comment 11 Deleted

My last version of this comment didn't make any sense, so I reworded it.

There is a relatively large amount of overhead for the overridden methods. A foreach over iteritems takes 7-8x the amount of time to complete compared to the builtin method -- 10k iteritems loops takes ~3s and ~22s for the builtin iteritems and mutable mapping iteritems, respectively, when the data is preloaded.

If we're not truly doing an iteritems, we have a very conservative break even point of about 1.5m fetches from the lazy loading config. I have a hard time imagining we'd even use 1% of that overhead on a small set of keys. When we actually need the full set of keys to load, we're looking at the ~0.07s of overhead to get the full data loaded plus the getitem overhead
Status: Started (was: Available)
Modifications to config_dump.json currently affect other processes -- the file is parsed by code outside of the chromite code.

Changing the code to only fetch the config when it's actually used rather than at the module level almost works, but at least one command currently requires site_config.params for building its argument parser. This ends up requiring the full config to load on every startup either way, so moving them out of the module level will end up having no effect. The instance I found is not one of the cros commands, it's scripts/gerrit.py, so it may be possible to isolate the argument parser building a bit more, but my guess right now is that'll be more trouble than it's worth in the long run.

The next best thing I've found seems to be in lazy parsing the configs. It does the load from the full file, but sets each key to the json decoded value rather than building the BuildConfig object, moving the BuildConfig object creation to be on use of the value. As with the last version, the defaults, templates, and params are all fully parsed as before.

This change drops the time for `cros lint -h` to about 0.55s. This is still a significant drop from the current implementation, but includes more overhead than the separate file implementation. Right now all tests pass, but I'm evaluating whether or not the tests actually properly cover the new code.

The params seem to be used frequently without anything else in the configs being used, so it seems as if moving those to a separate config so that the rest of the SiteConfig data doesn't need to be fetched to use the params would be another good approach. This doesn't feel like quite the right place to dig into doing that, though, and would require looking into the other process that uses the file.
The tests all pass and the save to string tests should be full coverage of the changes.
Project Member

Comment 15 by bugdroid1@chromium.org, Jul 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/a688799ade8a9e462a9d35060b3a06ed7b391c89

commit a688799ade8a9e462a9d35060b3a06ed7b391c89
Author: Mike Frysinger <vapier@chromium.org>
Date: Thu Jul 12 01:48:34 2018

config_lib: delay loading of site_config in many modules

We've gotten in the habit of assigning site_config in the global module
scope even if it's only used in that module in a few places.  We did
this historically for performance reasons (when GetConfig reparsed on
every call), but since that func now does its own caching, we no longer
need to take this up-front penalty.  So change many modules that only
use this config in a few places to call GetConfig on demand.

BUG= chromium:795449 
TEST=precq passes

Change-Id: I5df0a9d575c6f57cfb14e92b3c4fce480c39333e
Reviewed-on: https://chromium-review.googlesource.com/1130703
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Alex Klein <saklein@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>

[modify] https://crrev.com/a688799ade8a9e462a9d35060b3a06ed7b391c89/lib/request_build.py
[modify] https://crrev.com/a688799ade8a9e462a9d35060b3a06ed7b391c89/lib/triage_lib_unittest.py
[modify] https://crrev.com/a688799ade8a9e462a9d35060b3a06ed7b391c89/lib/gerrit.py
[modify] https://crrev.com/a688799ade8a9e462a9d35060b3a06ed7b391c89/lib/clactions.py
[modify] https://crrev.com/a688799ade8a9e462a9d35060b3a06ed7b391c89/lib/cq_config.py
[modify] https://crrev.com/a688799ade8a9e462a9d35060b3a06ed7b391c89/scripts/update_manifest_remotes.py
[modify] https://crrev.com/a688799ade8a9e462a9d35060b3a06ed7b391c89/lib/cros_test_lib.py
[modify] https://crrev.com/a688799ade8a9e462a9d35060b3a06ed7b391c89/lib/gclient.py

Project Member

Comment 16 by bugdroid1@chromium.org, Jul 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/aadbe9d540021454b54c0fbe55cf042389434c58

commit aadbe9d540021454b54c0fbe55cf042389434c58
Author: Mike Frysinger <vapier@chromium.org>
Date: Fri Jul 13 18:50:43 2018

cros_build_lib: drop unused ParseDurationToSeconds

This hasn't been used since 2014 when the last user was removed in
6e19265517aa18306c5f93b9a6eeebbcb285c91c.

BUG= chromium:795449 
TEST=precq passes

Change-Id: I59b9238930b4ca689cc538592886326e517ac95f
Reviewed-on: https://chromium-review.googlesource.com/1135301
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>

[modify] https://crrev.com/aadbe9d540021454b54c0fbe55cf042389434c58/lib/cros_build_lib.py
[modify] https://crrev.com/aadbe9d540021454b54c0fbe55cf042389434c58/lib/cros_build_lib_unittest.py

Project Member

Comment 17 by bugdroid1@chromium.org, Jul 15

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/1066629541674ce62f3eb9b9f34e2c453a0a08a2

commit 1066629541674ce62f3eb9b9f34e2c453a0a08a2
Author: Mike Frysinger <vapier@chromium.org>
Date: Sun Jul 15 01:11:48 2018

memoize: split out of cros_build_lib

The cros_build_lib module is much too large.  Split out the memoize
funcs so that we can use them in simpler modules w/out pulling in a
lot more stuff.

BUG= chromium:795449 
TEST=precq passes

Change-Id: I7ad60ae882402b5bca0be6fb0190a9c6a867258b
Reviewed-on: https://chromium-review.googlesource.com/1135302
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>

[modify] https://crrev.com/1066629541674ce62f3eb9b9f34e2c453a0a08a2/lib/cipd.py
[modify] https://crrev.com/1066629541674ce62f3eb9b9f34e2c453a0a08a2/cli/cros/cros_chrome_sdk.py
[modify] https://crrev.com/1066629541674ce62f3eb9b9f34e2c453a0a08a2/scripts/gerrit.py
[modify] https://crrev.com/1066629541674ce62f3eb9b9f34e2c453a0a08a2/lib/remote_access.py
[add] https://crrev.com/1066629541674ce62f3eb9b9f34e2c453a0a08a2/lib/memoize.py
[modify] https://crrev.com/1066629541674ce62f3eb9b9f34e2c453a0a08a2/lib/cros_build_lib.py
[modify] https://crrev.com/1066629541674ce62f3eb9b9f34e2c453a0a08a2/lib/path_util.py
[modify] https://crrev.com/1066629541674ce62f3eb9b9f34e2c453a0a08a2/lib/cgroups.py
[modify] https://crrev.com/1066629541674ce62f3eb9b9f34e2c453a0a08a2/lib/gob_util.py
[modify] https://crrev.com/1066629541674ce62f3eb9b9f34e2c453a0a08a2/scripts/cros_vm.py

Project Member

Comment 18 by bugdroid1@chromium.org, Jul 15

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/f9b822cd7957e0b5cbd4b2a591ed58e5945c1adf

commit f9b822cd7957e0b5cbd4b2a591ed58e5945c1adf
Author: Mike Frysinger <vapier@chromium.org>
Date: Sun Jul 15 11:25:37 2018

config_lib: use memoize helpers directly

This avoids the need for our own ad-hoc global caching.

BUG= chromium:795449 
TEST=precq passes

Change-Id: I8bdfc2fa6f0e6ad9ab27ea0ce02f9263757e58ca
Reviewed-on: https://chromium-review.googlesource.com/1135303
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>

[modify] https://crrev.com/f9b822cd7957e0b5cbd4b2a591ed58e5945c1adf/lib/config_lib_unittest.py
[modify] https://crrev.com/f9b822cd7957e0b5cbd4b2a591ed58e5945c1adf/lib/config_lib.py
[modify] https://crrev.com/f9b822cd7957e0b5cbd4b2a591ed58e5945c1adf/config/chromeos_config_unittest.py

To summarize the implemented changes, the site_param values are now all defined in the config_lib. The site params can now be accessed directly via a method in the config_lib, entirely bypassing the site config being built from the config_dump file when only the site params were needed. The result was `cros lint -h` down to ~0.4s from just over 1s.

The site params no longer need to be overridden at all. This means we can convert some of them to constants, but lib.cros_test_lib has some overrides for the GOB/GERRIT values that make solutions that don't involve manipulating global state a relatively large task, ask least from my research and attempts. IMO this is a task worth tackling at some point, and it should also clean up a fair bit of code (internal/external remote value checks and the like), but it might be better to just roll that into overhauling the configs as a whole.
Project Member

Comment 20 by bugdroid1@chromium.org, Jul 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/c265b743ecc949d1ab2f65b6fe602d26264abc21

commit c265b743ecc949d1ab2f65b6fe602d26264abc21
Author: Alex Klein <saklein@chromium.org>
Date: Sat Jul 21 01:33:58 2018

Moved all site_param values into config_lib defaults.

Added direct accessor for site param defaults to circumvent loading
config_dump.json for site param values.

Removed all site param "overrides" in chromeos configs.

Updated config_dump - _site_params now empty because all values are
default values.

BUG= chromium:795449 
TEST=run_tests
CQ-DEPEND=CL:1144202

Change-Id: I59e4ad0387e70b30a9748bd0d01a93d5351ad978
Reviewed-on: https://chromium-review.googlesource.com/1144201
Commit-Ready: Alex Klein <saklein@chromium.org>
Tested-by: Alex Klein <saklein@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>

[modify] https://crrev.com/c265b743ecc949d1ab2f65b6fe602d26264abc21/config/chromeos_config.py
[modify] https://crrev.com/c265b743ecc949d1ab2f65b6fe602d26264abc21/lib/config_lib.py
[modify] https://crrev.com/c265b743ecc949d1ab2f65b6fe602d26264abc21/config/config_dump.json
[modify] https://crrev.com/c265b743ecc949d1ab2f65b6fe602d26264abc21/config/chromeos_config_unittest.py

Project Member

Comment 21 by bugdroid1@chromium.org, Jul 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/2ab29cc5fd1440973d590447615072add592f525

commit 2ab29cc5fd1440973d590447615072add592f525
Author: Alex Klein <saklein@chromium.org>
Date: Sat Jul 21 01:33:59 2018

Replaced most site_config.params usages with new direct access.

BUG= chromium:795449 
TEST=run_tests

Change-Id: Icb7d9700fa8c1b77ef97bf47fb1e096baa09da36
Reviewed-on: https://chromium-review.googlesource.com/1144202
Commit-Ready: Alex Klein <saklein@chromium.org>
Tested-by: Alex Klein <saklein@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/lib/gerrit.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/commands.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/stages/branch_stages.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cli/cros/cros_tryjob.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/repository_unittest.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cli/cros/cros_pinchrome.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/stages/report_stages.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/validation_pool.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/lib/gob_util_unittest.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/relevant_changes.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/lkgm_manager_unittest.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/manifest_version.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/lib/patch.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/lib/git.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/lkgm_manager.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/scripts/gerrit.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/lib/clactions.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/scripts/cros_portage_upgrade.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/lib/triage_lib_unittest.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/build_status_unittest.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/scripts/update_manifest_remotes.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/scripts/cbuildbot_launch.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/lib/builder_status_lib.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/trybot_patch_pool.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/lib/cros_test_lib.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/lib/gerrit_unittest.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cli/cros/cros_uprevchrome.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/trybot_patch_pool_unittest.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/stages/sync_stages.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/repository.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/patch_series_unittest.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/lib/gclient.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/patch_series.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/lib/patch_unittest.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/validation_pool_unittest.py
[modify] https://crrev.com/2ab29cc5fd1440973d590447615072add592f525/cbuildbot/commands_unittest.py

we're down to ~600ms now.  i'd say that's on the edge of what is acceptable, but we might not have too many low hanging fruits at this point.

`time gerrit --help` is about 100ms faster, so looks like cros has a specific slowdown from loading all the cros_xxx.py modules.
My machine runs it in about 430ms, but it definitely is running out of low hanging fruit. I attached a visualized call graph that has the major components now, and labeled 3 of the branches with red text.

"Third Party" is third_party/infra_libs, and then "re lib" and "pkg_resources" are the python regular expression library, used in a lot of places, and the pkg_resources library used in the chromite __init__.py. 

Just the pkg_resources init alone takes nearly half the time, and when including the infra_libs we're up to about 70% of the total run time. Depending on where the re calls are, that could bring the total up to 80+%.

I'd guess there's probably some places that could address re calls in our own code (e.g. moving re.compile calls out of loops), but the other two are both not our code and called only once, so they're going to be a lot harder to address.

I also get gerrit --help running faster, about 70ms faster for me. It has similar numbers for infra_lib, ~20ms improvement for pkg_resources, and only ~10ms improvement for the re calls.
cros_lint_h.png
306 KB View Download
Status: Fixed (was: Started)
There'll always be more to do for optimizations, but considering the progress this seems worth marking as fixed.

Sign in to add a comment