Skip to content
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

Remove lru_cache from get_active_msgids #5672

Merged
merged 1 commit into from
Jan 13, 2022

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Jan 13, 2022

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

Ref #5670.

Instead of using lru_cache we can use a basic cache defined on the instance to achieve a similar performance boost.

Tested on pylint over pylint.
With new form of cache:

ncalls  tottime  percall  cumtime  percall filename:lineno(function)
54436    0.029    0.000    0.030    0.000 message_id_store.py:107(get_active_msgids)

Without new cache:

ncalls  tottime  percall  cumtime  percall filename:lineno(function)
54429    0.099    0.000    0.132    0.000 message_id_store.py:107(get_active_msgids)

Problem introduced in #5605

@DanielNoord DanielNoord added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Jan 13, 2022
@DanielNoord DanielNoord added this to the 2.13.0 milestone Jan 13, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1692803372

  • 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 93.831%

Totals Coverage Status
Change from base Build 1692435585: 0.002%
Covered Lines: 14647
Relevant Lines: 15610

πŸ’› - Coveralls

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.

Looks good, I did not understand the performance report in the description though. No cache was actually faster than using lru cache ?!

@DanielNoord
Copy link
Collaborator Author

Seems I miswrote the description. Updated.

The first is with this change, the second is without any form of cache. I haven't included lru_cache as that's not really a viable option.

@Pierre-Sassoulas Pierre-Sassoulas merged commit e75e37a into pylint-dev:main Jan 13, 2022
@DanielNoord DanielNoord deleted the lru-cache branch January 13, 2022 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants