Cache splitnode results to improve tests collection time

[Nepherhotep]

#5516

On a large number of tests, collection time can take over 2 minutes for 20k of tests. 60% of the time is spent on splitting nodes, so caching nodesplit results will improve performance considerably.

[codecov]

Codecov Report

Merging #5681 into master will decrease coverage by 0.2%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5681      +/-   ##
==========================================
- Coverage   96.12%   95.91%   -0.21%     
==========================================
  Files         117      117              
  Lines       25796    25797       +1     
  Branches     2495     2495              
==========================================
- Hits        24796    24744      -52     
- Misses        695      744      +49     
- Partials      305      309       +4

Impacted Files	Coverage Δ
src/_pytest/nodes.py	95.54% <100%> (+0.02%)	⬆️
src/_pytest/pytester.py	85.03% <0%> (-5.93%)	⬇️
testing/test_assertrewrite.py	83.26% <0%> (-1.42%)	⬇️
src/_pytest/terminal.py	92.6% <0%> (-0.43%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a5e72c...b43ebb7. Read the comment docs.

[asottile]

a cache of mutable return values is asking for trouble -- if this returned tuples I'd be more comfortable

[nicoddemus]

Yeah, it would be safer. @Nepherhotep would you mind changing that so we can see how it fares?

[Nepherhotep]

@nicoddemus, a simple converting response format from list to tuple caused 1500 errors. Most of them failed with the error "fixture testdir not found", some of them failed with repr error.
I need to dig into "testdir" fixture issue a bit deeper.

[nicoddemus]

I'm surprised... strange.

[nicoddemus]

Oh you missed the early return:

    if nodeid == "":
        # If there is no root node at all, return an empty list so the caller's logic can remain sane
        return []

So sometimes it would return a tuple, others a list.

I've reproduced it locally, returning a tuple at both places worked fine here.

[Nepherhotep]

Ah, my bad, thanks for pointing to that!

[nicoddemus]

Hi @Nepherhotep,

Thanks a lot for this!

I also would like to apologize for not following up on #5516, I couldn't find the time yet.

[nicoddemus]

Great work, thanks!

Please also include a CHANGELOG entry, I suggest:

Cache internal node splitting function which can improve collection performance in very large test suites.

In a changelog/5516.trivial.rst file.

Thanks!

[asottile]

@Nepherhotep can you show the difference in cpu time / memory consumption? lru_cache(maxsize=None) is ~usually a memory leak -- want to make sure the trade offs are correct here or if we should be looking to refactor the code such that it doesn't do as many repeated lookups instead

[Nepherhotep]

@Nepherhotep can you show the difference in cpu time / memory consumption? lru_cache(maxsize=None) is ~usually a memory leak -- want to make sure the trade offs are correct here or if we should be looking to refactor the code such that it doesn't do as many repeated lookups instead.

@asottile For ~15k tests, collection time decreases from 130 to 90 seconds (i7 8-gen CPU), as for memory, I don't have this information right away. I'll try to collect it.
I don't think memory increase is substantial - the problem is in splitting the same node path multiple times, but the number of nodes is relatively small.

[nicoddemus]

I don't think memory increase is substantial - the problem is in splitting the same node path multiple times

I agree, the cache for this function should get equal to the number of nodeids in the section, and that number is fixed and relatively small.

@asottile do you still have concerns?

[asottile]

no particular concern with the memory -- was just curious about the affect.

[nicoddemus]

Great, thanks! Would you mind squashing the commits before merging? Or I would be happy to do it myself if you let me. 👍

[Nepherhotep]

@nicoddemus done.
I wonder, though, why don't you configure GitHub project to do "Squash and Merge" pull request by default - it's a very convenient setup :)

[nicoddemus]

@asottile doesn't like it, IIRC.