Skip to content

Limit expensive decorator function #1407

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 27, 2022

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Feb 27, 2022

Description

Modeled after the idea here, replace the deprecate_default_argument_values decorator with a default one if DeprecationWarnings aren't enabled. I.e. these will still show during Pytest runs or if a user chooses to enable warnings (with -Wd or similar).

Github Actions isn't the most reliable / reproducible, but I think it's about a 10-30% improvement.

@DanielNoord You did the initial performance analysis in #1157 (comment). Would you be able to check again with these changes? Just a heads up, I don't know if the profiler enables DeprecationWarnings. Just for testing, you could add a simple print inside the if block to make sure.

https://docs.python.org/3/library/warnings.html#default-warning-filter
https://docs.python.org/3/using/cmdline.html#cmdoption-W

Closes #1383

@cdce8p cdce8p modified the milestones: 2.11.0, 2.10.0 Feb 27, 2022
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice way to keep the deprecation warning when it matters.

But it means when deprecation warnings are activated in user code astroid will still be slow right ? Maybe we should add a warning about that in the changelog ?

@DanielNoord
Copy link
Collaborator

DanielNoord commented Feb 27, 2022

@DanielNoord You did the initial performance analysis in #1157 (comment). Would you be able to check again with these changes? Just a heads up, I don't know if the profiler enables DeprecationWarnings. Just for testing, you could add a simple print inside the if block to make sure.

FYI, I checked and with python -m cProfile the profiler does not enable DeprectationWarnings. I'm obviously unsure if there are specifics about other ways of invoking the profiler.

This is indeed much better! I did a rough profile and on a successive run over pylint itself I went from 64 to 53 seconds. I'm always unsure how to calculate this but 53/64 = 0,82. So I guess you could say almost a 20% performance increase?
Of course this is all very basic evidence, but this definitely solves the underlying issue. Thanks for looking into this @cdce8p!

@cdce8p
Copy link
Member Author

cdce8p commented Feb 27, 2022

But it means when deprecation warnings are activated in user code astroid will still be slow right ? Maybe we should add a warning about that in the changelog ?

I've already mentioned it in the Changelog message.

Only run expensive decorator functions when
non-default Deprecation warnings are enabled, eg. during a Pytest run.

The change itself doesn't make astroid slower than it's already. It just is an improvement if Deprecation warnings are disabled.

@cdce8p cdce8p merged commit 8f7f078 into pylint-dev:main Feb 27, 2022
@cdce8p cdce8p deleted the performance-decorator branch February 27, 2022 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix performance issue with inspect calls in deprecation warnings decorators
3 participants