owners.py: partial fix for owners-check perf regression

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
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/7e16cf303221bbcf81d632924e19ddc888da9c3b

[ncarter (slow), 2016-08-30 22:51:26]

nick@chromium.org changed reviewers:
+ dpranke@google.com, dtu@chromium.org

[ncarter (slow), 2016-08-30 22:51:27]

This is still experimental, and i don't fully understand how owners.py works,
just hoping to have a discussion here.

https://codereview.chromium.org/2293233002/diff/1/owners.py
File owners.py (right):

https://codereview.chromium.org/2293233002/diff/1/owners.py#newcode184
owners.py:184: while not self._owners_for(dirpath):
I wonder if this check needs something like what happened with
load_data_needed_for().

It seems odd to ever call _owners_for() on a dirname (versus a full file path),
since the glob rules in OWNERS files are meant to pertain to files, not dirs.

[dtu, 2016-08-31 02:35:46]

Forgive me if I don't really know what's going on with this code either -- I
think it's been a while since anyone has looked at it :)

Thankfully, I think the unit tests in the tests/ directory are decent.

https://codereview.chromium.org/2293233002/diff/1/owners.py
File owners.py (right):

https://codereview.chromium.org/2293233002/diff/1/owners.py#newcode194
owners.py:194: while dirpath not in loaded_dirs:
Is loaded_dirs supposed to be at the top-level? I think as written it runs until
loaded_dirs becomes '', so this is the same as doing "while dirpath:"

"while dirpath:" would still be correct here. If it's faster than the current
check, then that's an easy change to make for both this function and
_enclosing_dir_with_owners.

[ncarter (slow), 2016-09-01 21:58:15]

Please take another look.

It turns out that just caching the fnmatch regexes suffices to get most of the
perf back (it gets us back to ~600ms for my original CL, which is probably okay
ish for a large CL).

There are opportunities to speed that up some more, but I encountered some
correctness issues with the existing code (bug 643390) that we should fix first.

[ncarter (slow), 2016-09-01 21:58:34]

Description was changed from

==========
owners.py: partial fix for owners-check perf regression

load_data_needed_for() doesn't need seem to need to do glob computations
it's just trying to hit each OWNERS file once.

BUG=
==========

to

==========
owners.py: partial fix for owners-check perf regression

load_data_needed_for() doesn't need seem to need to do glob computations
it's just trying to hit each OWNERS file once.

BUG=642793
==========

[ncarter (slow), 2016-09-01 21:59:08]

Description was changed from

==========
owners.py: partial fix for owners-check perf regression

load_data_needed_for() doesn't need seem to need to do glob computations
it's just trying to hit each OWNERS file once.

BUG=642793
==========

to

==========
owners.py: partial fix for owners-check perf regression

fnmatch.fnmatch seems to fall off a performance cliff once you start exhausting
an internal cache.

BUG=642793
==========

[ncarter (slow), 2016-09-01 21:59:41]

Description was changed from

==========
owners.py: partial fix for owners-check perf regression

fnmatch.fnmatch seems to fall off a performance cliff once you start exhausting
an internal cache.

BUG=642793
==========

to

==========
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
==========

[dtu, 2016-09-01 23:31:55]

lgtm

[ncarter (slow), 2016-09-01 23:38:07]

The CQ bit was checked by nick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 23:38:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 23:44:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 23:44:41]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-09-08 17:07:39]

dpranke: ping

[Dirk Pranke, 2016-09-16 22:57:33]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2016-09-16 22:57:34]

I'm sorry for the delay in this. I was OOO for a while, and then I was busy
getting caught up after being out and procrastinating b/c I thought this change
would be hard; I didn't realize you had simplified things so much.

This LGTM. Thanks!

[ncarter (slow), 2016-09-16 22:59:30]

No worries, we've all been there.

[ncarter (slow), 2016-09-16 22:59:34]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2016-09-16 22:59:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 23:05:07]

Description was changed from

==========
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
==========

to

==========
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

Committed:
https://chromium.googlesource.com/chromium/tools/depot_tools/+/7e16cf303221bb...
==========

[commit-bot: I haz the power, 2016-09-16 23:05:09]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/tools/depot_tools/+/7e16cf303221bb...