Performance regression: `pointless-exception-statement`

[cdce8p]

Thanks for the new check! I've just tested it with Home-Assistant and was able to find a handful of issues. Unfortunately, the current implementation has significant performance problems.

Comparing 5612134 with 89a20e2, a full pylint ci run can take between 6% and 43% more time. The spread is likely a result of different caches as -j2 is used for the run.

before: 7:59
after: 8:29 - 11:35 (multiple runs)

Originally posted by @cdce8p in #7939 (review)

[cdce8p]

Cross posting possible solutions from #7939 (comment)

Inference can be expensive, especially if the lru cache has reached is maxsize and old entries are overwritten.

We should not call for safe_infer for every Call node. Instead I would recommend to use a heuristic and filter likely candidates first.

• Almost all builtin error names contain either Exception, Error, or Warning. That likely applies to custom exceptions as well. https://docs.python.org/3/library/exceptions.html#exception-hierarchy
• This could be further improved with a check against other predefined names. For the stdlib, those would probably be KeyboardInterrupt, StopAsyncIteration, StopIteration.
• Lastly a new config option could be added to allow the user to specify additional names (maybe even regex).

The goal IMO should be to capture 95% of the likely errors without the huge performance impact.

[jayaddison]

I'm not yet able to replicate the reported performance degradations locally, when running the comparative versions of the pylint codebase (with and without #7939) on the codebase for astroid:

The benchmark process I'm using is:

pylint.git $ python3 -m venv .venv
pylint.git $ source .venv/bin/activate
pylint.git $ pip install -r requirements_test_min.txt
pylint.git $ export PYTHONPATH=.

pylint.git $ git checkout 89a20e2ccdb2927ac370f7c6663fc83318988e10
pylint.git $ rm -rf ~/.cache/pylint/

pylint.git $ time find .venv/lib/python3.*/site-packages/astroid/ -name '*.py' -exec python3 -m pylint -j2 --output /dev/null {} +

pylint.git $ git checkout 56121344f6c9c223d8e4477c0cdb58c46c840b4b
pylint.git $ rm -rf ~/.cache/pylint/

pylint.git $ time find .venv/lib/python3.*/site-packages/astroid/ -name '*.py' -exec python3 -m pylint -j2 --output /dev/null {} +

[cdce8p]

I'm not yet able to replicate the reported performance degradations locally, when running the comparative versions of the pylint codebase (with and without #7939) on the codebase for astroid

The codebases are likely too small to see any impact. We do cache the results of safe_infer but only the last 1024. I believe someone reported a memory issue some time ago which is why we limited the cache size.

If more calls are inferred and the cache is overwritten, inference needs to happen all over again. Combined with the non-deterministic behavior of -j2 (files are not linted in a consistent order), it would explain the large time increases.

You could try to reduce the cache size here and see if the impact becomes noticeable
https://github.com/PyCQA/pylint/blob/ee55ec8c17f525c0a35bae710a4b27ab591d688f/pylint/checkers/utils.py#L1350-L1357

[jayaddison]

Maybe off-topic, but, while I'm experimenting with that: does increasing the cache size improve performance when pylint runs on the Home Assistant codebase?

[jayaddison]

Ok, I do see some performance degradation for #7939 when compared against the previous commit on a larger codebase; I'll make some adjustments to the original pull request to see whether they improve the situation.

pylint.git $ git checkout 89a20e2ccdb2927ac370f7c6663fc83318988e10
HEAD is now at 89a20e2cc Add new check "pointless-exception-statement" (#7939)

pylint.git $ time find home-assistant/core.git/ -name '*.py' -exec python3 -m pylint -j2 --output /dev/null {} +

real    12m33.726s
user    23m25.588s
sys     0m47.887s

pylint.git $ rm -rf ~/.cache/pylint/
pylint.git $ git checkout 56121344f6c9c223d8e4477c0cdb58c46c840b4b
HEAD is now at 56121344f [pre-commit.ci] pre-commit autoupdate (#8016)

pylint.git $ time find home-assistant/core.git/ -name '*.py' -exec python3 -m pylint -j2 --output /dev/null {} +

real    11m59.741s
user    22m58.198s
sys     0m46.425s

[jayaddison]

Maybe off-topic, but, while I'm experimenting with that: does increasing the cache size improve performance when pylint runs on the Home Assistant codebase?

It doesn't appear to, from an initial inspection: with an lru_cache of maxsize=8192 on safe_infer...

$ rm -rf ~/.cache/pylint/
$ time find home-assistant/core.git/ -name '*.py' -exec python3 -m pylint -j2 --output /dev/null {} + 
real	12m2.359s
user	23m1.790s
sys	0m46.339s

[jayaddison]

A naive implementation of suffix-based name filtering before inference is applied doesn't appear to help much, if at all:

-            inferred = utils.safe_infer(expr)
+            inferred = None
+            if any(
+                expr.func.name.endswith(suffix)
+                for suffix in ("Error", "Exception", "Warning")
+            ):
+                inferred = utils.safe_infer(expr)

$ time find home-assistant/core.git/ -name '*.py' -exec python3 -m pylint -j2 --output /dev/null {} + 
real	12m0.502s
user	22m57.999s
sys	0m46.789s

[jayaddison]

@cdce8p I'm unsure how to proceed with this - I was attempting to replicate and verify that performance is better without #7939 and that performance is degraded after it was merged, but I haven't managed to achieve that.

I believe that there may be a performance regression but I'm not certain about the conditions required for it to appear.

What can we do to find it?

[jayaddison]

What can we do to find it?

I'm planning to run some more testing with the LRU cache size reduced, as suggested, to see if that helps track down the cause.

[cdce8p]

Maybe off-topic, but, while I'm experimenting with that: does increasing the cache size improve performance when pylint runs on the Home Assistant codebase?

It doesn't appear to, from an initial inspection: with an lru_cache of maxsize=8192 on safe_infer...

Saw the same. Setting maxsize=None actually resulted in a performance hit. 8:00 -> 9:38min
Would need to investigate this, but might be astroid caching then, not pylint.

--

A naive implementation of suffix-based name filtering before inference is applied doesn't appear to help much, if at all:

-            inferred = utils.safe_infer(expr)
+            inferred = None
+            if any(
+                expr.func.name.endswith(suffix)
+                for suffix in ("Error", "Exception", "Warning")
+            ):
+                inferred = utils.safe_infer(expr)

Took me a moment to verify but a naive implementation does fix the CI issue. Time is back down to 8:00.
However, I needed to adjust your snipped a bit as it crashed immediately for me. Turns out Attribute nodes don't have name but use attrname instead.

The change I tested: 46c8a97

It probably needs to be something like this

        if isinstance(expr, nodes.Call):
            inferred = None
            name = ""
            if isinstance(expr.func, nodes.Name):
                name = expr.func.name
            elif isinstance(expr.func, nodes.Attribute):
                name = expr.func.attrname
            if any(
                name.endswith(suffix)
                for suffix in ("Error", "Exception", "Warning")
            ):
                inferred = utils.safe_infer(expr)

[jayaddison]

Saw the same. Setting maxsize=None actually resulted in a performance hit. 8:00 -> 9:38min

Took me a moment to verify but a naive implementation does fix the CI issue. Time is back down to 8:00.

Interesting, and good to hear, respectively - thanks for checking both of those!

I'm going to spend some time experimenting with your fix - feel free to open that as a pull request, if you like?

Also, but low priority: if you have time for any further testing: could you check whether #8075 also solves the problem? (it's a different approach -- and I don't think it's great -- but it could be useful to know whether it's valid)