Remove hook proxy cache

[nicoddemus]

Fixes #2016

@hpk42 I would appreciate if you could take a look as you probably know the most about that part of the code. This comment contains the full description of my diagnosis of the problem.

cc @d-b-w

[coveralls]

Coverage decreased (-0.004%) to 92.678% when pulling e1a75bd on nicoddemus:fix-hookproxy-cache into 7574033 on pytest-dev:features.

[d-b-w]

I think that your approach looks clever & correct. But I do want to double check, given that this is an increase in complexity - did you consider just removing the _fs2hookproxy cache? My (very limited) experiments show that the cache doesn't improve performance at all.

[d-b-w]

This doesn't seem wrong or anything, but is it actually relevant to this code change?

[nicoddemus]

No, just that I needed this to make the test easier. Now one can use sub-directories in the keys to testdir.makepyfile (actually any "make*" functions):

testdir.makepyfile(**{
    'foo/bar/foobar.py': 'print("hello world")',
})

[nicoddemus]

But I do want to double check, given that this is an increase in complexity - did you consider just removing the _fs2hookproxy cache? My (very limited) experiments show that the cache doesn't improve performance at all.

I did consider, and I thought that since gethookproxy is called for each entry in a directory it would impact collection considerably.

So I decided to make a benchmark to see if it was true. I wrote this script which creates an arbitrarily nested directory tree with tests on it. Running:

generate_tests_tree.py 4 6 10 0

Generates a tree 4 levels deep, 6 folders per level and 10 test files each, without any conftest.py files (that's what the last 0 means). Each test file contains 10 dummy tests, which just pass.

Here are the results I got on my Windows box:

no-conftests:
-------------

python generate_tests_tree.py 4 10 5 0
2000 tests
cache: 2,0055171
no-cache: 2,0317655


python generate_tests_tree.py 4 5 10 0 
2000 tests
cache: 1,9863132
no-cache: 2,0168685

python generate_tests_tree.py 10 10 30 0
30000 tests
cache: 16,5217091
no-cache: 16,9246796


with conftests
--------------

python generate_tests_tree.py 4 10 5 1
2000 tests
cache: 2,1642561
no-cache: 2,4153508

python generate_tests_tree.py 4 5 10 1
2000 tests
cache: 2,0458092
no-cache: 2,1860023 

python generate_tests_tree.py 10 10 30 1
30000 tests
cache: 18,6366042
no-cache: 27,1169733

The timings were obtained by running pytest --collect-only in the generated directory and measuring that using powershell.

So it seems there's some gain by using the cache (specially when there are conftests), but they see minimal in my opinion. That 9-10s difference with 30000 tests doesn't seem relevant, after all with 30000 tests that test suite, if it were real, would take several minutes to run (if not hours). It takes 76s to run the full suite, and those are all tests with just pass.

Unless there's something wrong with my testing, I agree that we could just take out that cache.

[coveralls]

Coverage increased (+0.007%) to 92.689% when pulling 56e8274 on nicoddemus:fix-hookproxy-cache into 5ce551e on pytest-dev:features.

[d-b-w]

Cool! Really nice experiment.

[hpk42]

great analysis. This really calls for removing the cache. If a caching layer does not provide measurable benefit it should go. There are two hard CS problems: naming, caching and off-by-one errors.

[nicoddemus]

Thanks @hpk42! I will update the PR to remove the cache then. 👍

[nicoddemus]

Changed target to master because the change now is perfectly safe.

[coveralls]

Coverage increased (+0.008%) to 92.833% when pulling 81528ea on nicoddemus:fix-hookproxy-cache into 64193ad on pytest-dev:master.

[coveralls]

Coverage increased (+0.008%) to 92.833% when pulling 81528ea on nicoddemus:fix-hookproxy-cache into 64193ad on pytest-dev:master.