-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Refactor message disabling and enabling #5596
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
Conversation
Pull Request Test Coverage Report for Build 1628212835
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, this is great thank you ! Small question to maybe trim a sorting on top of what was already done.
pylint/lint/pylinter.py
Outdated
# sync configuration object | ||
msgs = self._msgs_state | ||
self.config.enable = [ | ||
self._message_symbol(mid) for mid, val in sorted(msgs.items()) if val |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering why we sort here. If it's in order to be able to test the class, we could sort in the test directly (where it won't affect our run time performance).
@Pierre-Sassoulas Could you add a suggestion and see if it works without sorting? (I'm on mobile) |
The change seems to work locally but I can't use the profiling command since we added
Any idea how to solve this ? |
I made a change without checking that the performance got improved. (Theoretically it should be better, but...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I found a way to test performance, and the change I made is actually a tad better:
git rebase origin/main -i --exec="python -m cProfile -s cumtime -m pylint --disable=all test.py|grep config_initialization"
Executing: python -m cProfile -s cumtime -m pylint --disable=all test.py|grep config_initialization
1 0.000 0.000 0.163 0.163 config_initialization.py:14(_config_initialization)
1 0.000 0.000 0.000 0.000 config_initialization.py:4(<module>)
Executing: python -m cProfile -s cumtime -m pylint --disable=all test.py|grep config_initialization
1 0.000 0.000 0.014 0.014 config_initialization.py:14(_config_initialization)
1 0.000 0.000 0.000 0.000 config_initialization.py:4(<module>)
Executing: python -m cProfile -s cumtime -m pylint --disable=all test.py|grep config_initialization
1 0.000 0.000 0.012 0.012 config_initialization.py:14(_config_initialization)
1 0.000 0.000 0.000 0.000 config_initialization.py:4(<module>)
Successfully rebased and updated refs/heads/performance.
Successfully rebased and updated refs/heads/performance.
I think we can merge, let me know whay you think @DanielNoord
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Type of Changes
Description
Closes #5587.
This is the main improvement I found for enabling and disabling messages. The problem was that we synced the
config
object after every message, instead of doing so after all messages have been set.With this version we first find all messages to set, set them and then sync the
config
object.For comparison:
Profiler command:
python -m cProfile -s cumtime pylint/__main__.py test.py --disable=all
Without change:
With change:
About a 10x improvement 😄