Skip to content
New issue

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

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

Already on GitHub? # to your account

Optimize is_trailing_comma() #8606

Merged

Conversation

jacobtylerwalls
Copy link
Member

Type of Changes

Type
🔨 Refactoring

Description

Refs #5534

When profiling a lint over pygame, I see a savings of over 2s on my M1 Mac/Python3.11:

Before
ncalls tottime percall cumtime percall filename:lineno(function)
493189 3.580 0.000 3.640 0.000 refactoring_checker.py:88(_is_trailing_comma)

After
ncalls tottime percall cumtime percall filename:lineno(function)
493189 1.346 0.000 1.400 0.000 refactoring_checker.py:88(_is_trailing_comma)

@jacobtylerwalls jacobtylerwalls added performance Skip news 🔇 This change does not require a changelog entry labels Apr 23, 2023
@jacobtylerwalls jacobtylerwalls added this to the 3.0.0b1 milestone Apr 23, 2023
@codecov
Copy link

codecov bot commented Apr 23, 2023

Codecov Report

Merging #8606 (e1e5643) into main (6be842e) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8606      +/-   ##
==========================================
- Coverage   95.91%   95.91%   -0.01%     
==========================================
  Files         174      174              
  Lines       18417    18416       -1     
==========================================
- Hits        17665    17664       -1     
  Misses        752      752              
Impacted Files Coverage Δ
pylint/checkers/refactoring/refactoring_checker.py 98.34% <100.00%> (-0.01%) ⬇️

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.

Great ! Reading the MR you linked was entertaining: we increased the timeout on it and did no deep analysis of the performance. I suppose we were reaching the timeout regularly before and this was the straw that made us reach the timeout too much. Motivation to add benchmark regression primers (discussed in the issue) reactivated :D

@jacobtylerwalls
Copy link
Member Author

Thanks for the review! I have an additional commit to push to this branch, but wanted to wait for the primer 3.7 job to finish first (hopefully it's almost done!)--just to make sure it finishes reasonably.

@github-actions

This comment has been minimized.

@jacobtylerwalls jacobtylerwalls removed the Skip news 🔇 This change does not require a changelog entry label Apr 23, 2023
@github-actions

This comment has been minimized.

@jacobtylerwalls jacobtylerwalls requested review from Pierre-Sassoulas and removed request for DanielNoord April 23, 2023 17:45
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.

Great ! The second optimization with self.linter.is_message_enabled( would be huge if used everywhere, but this would require design to be done right imo (@utils.only_required_for_messages working for token based checker and a check to verify that it's used when required and not used when not required ?)

@github-actions
Copy link
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit e1e5643

Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request May 8, 2024
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request May 13, 2024
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request May 13, 2024
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request May 13, 2024
Performance analysis by correctmost.  Follow up to fix the known limitation will be in pylint-dev#9609

Refs pylint-dev#8606 (follow-up)
Closes pylint-dev#9608
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request May 13, 2024
Performance analysis by correctmost.  Follow up to fix the known limitation will be in pylint-dev#9609

Refs pylint-dev#8606 (follow-up)
Closes pylint-dev#9608
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants