Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
gh-105623 Fix performance degradation in logging RotatingFileHandler#105887
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…ndlerThe check for whether the log file is a real file is expensive on NFSfilesystems. This commit reorders the rollover condition checking tonot do the file type check if the expected file size is less than therotation threshold.
ghost commentedJun 17, 2023 • edited by ghost
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by ghost
Uh oh!
There was an error while loading.Please reload this page.
All commit authors signed the Contributor License Agreement. |
Misc/NEWS.d/next/Library/2023-06-17-09-07-06.gh-issue-105623.5G06od.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
…G06od.rstCo-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
bedevere-bot commentedJun 19, 2023
From buildbots: This PR is not the cause (Python-only change cannot break reference counting). However, we need to fix the crash before merging the PR. |
| # See bpo-45401: Never rollover anything other than regular files | ||
| if os.path.exists(self.baseFilename) and not os.path.isfile(self.baseFilename): | ||
| return False |
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.
Even with a local file-system this is a substantive chunk of the run time
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.
@bdraco I'm not familiar with the report generated by py-spy. Are you seeing that the os.path.exists() and os.path.isfile() are expensive or the self.format()? The self.format() is probably a different bug that needs fixed since rollover is formatting the message once to get its length and then emit later formats it again.
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'd expect the format and emit to be expensive here (relatively ... the logger is quite busy). What is unexpected is the cost ofos.path.exists andos.path.isfile which is solved by this PR.
Sorry for the lack of clarity. This looks like a good fix and I wanted to point out the problem is not limited to NFS.
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.
Under normal usage,self.baseFilename doesn't change. Perhaps we just cache the result of the existence/is-file test when we open the file? Wouldn't that be a better approach?
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. In the current paradigm it is the right solution.
I wonder whether it would be better to perform this check indoRollover(), before renaming files. But this is a different and not so easy issue.
fantabolous commentedJun 27, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks, in my rather logging-heavy Windows NTFS project, shouldRollover went from 25-35% of cpu to <2% after monkey patching this in. (According to yappi.) |
e9b4ec6 intopython:mainUh oh!
There was an error while loading.Please reload this page.
Thanks@zhatt for the PR, and@serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12. |
Thanks@zhatt for the PR, and@serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
…ndler (pythonGH-105887)The check for whether the log file is a real file is expensive on NFSfilesystems. This commit reorders the rollover condition checking tonot do the file type check if the expected file size is less than therotation threshold.(cherry picked from commite9b4ec6)Co-authored-by: Craig Robson <craig@zhatt.com>Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
…ndler (pythonGH-105887)The check for whether the log file is a real file is expensive on NFSfilesystems. This commit reorders the rollover condition checking tonot do the file type check if the expected file size is less than therotation threshold.(cherry picked from commite9b4ec6)Co-authored-by: Craig Robson <craig@zhatt.com>Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
GH-121116 is a backport of this pull request to the3.12 branch. |
GH-121117 is a backport of this pull request to the3.13 branch. |
Thank you for your contribution@zhatt, and thank you for reminder@fantabolous. I forgot about this PR. |
…andler (GH-105887) (GH-121116)The check for whether the log file is a real file is expensive on NFSfilesystems. This commit reorders the rollover condition checking tonot do the file type check if the expected file size is less than therotation threshold.(cherry picked from commite9b4ec6)Co-authored-by: Craig Robson <craig@zhatt.com>Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
…andler (GH-105887) (GH-121117)The check for whether the log file is a real file is expensive on NFSfilesystems. This commit reorders the rollover condition checking tonot do the file type check if the expected file size is less than therotation threshold.(cherry picked from commite9b4ec6)Co-authored-by: Craig Robson <craig@zhatt.com>Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
…ndler (pythonGH-105887)The check for whether the log file is a real file is expensive on NFSfilesystems. This commit reorders the rollover condition checking tonot do the file type check if the expected file size is less than therotation threshold.Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
…ndler (pythonGH-105887)The check for whether the log file is a real file is expensive on NFSfilesystems. This commit reorders the rollover condition checking tonot do the file type check if the expected file size is less than therotation threshold.Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
…ndler (pythonGH-105887)The check for whether the log file is a real file is expensive on NFSfilesystems. This commit reorders the rollover condition checking tonot do the file type check if the expected file size is less than therotation threshold.Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
Uh oh!
There was an error while loading.Please reload this page.
The check for whether the log file is a real file is expensive on NFS filesystems. This commit reorders the rollover condition checking to not do the file type check if the expected file size is less than the rotation threshold.