gh-105623 Fix performance degradation in logging RotatingFileHandler#105887
Uh oh!
There was an error while loading. Please reload this page.
Conversation
…ndler 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.
All commit authors signed the Contributor License Agreement. |
Misc/NEWS.d/next/Library/2023-06-17-09-07-06.gh-issue-105623.5G06od.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
…G06od.rst Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
vsajip added the 🔨 test-with-buildbots 
bedevere-bot commented Jun 19, 2023
bedevere-bot removed the 🔨 test-with-buildbots 
arhadthedev commented Jun 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 | ||
| ifos.path.exists(self.baseFilename) andnotos.path.isfile(self.baseFilename): | ||
| returnFalse |
There was a problem hiding this comment.
Even with a local file-system this is a substantive chunk of the run time
There was a problem hiding this comment.
@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.
I'd expect the format and emit to be expensive here (relatively ... the logger is quite busy). What is unexpected is the cost of os.path.exists and os.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.
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?
serhiy-storchaka left a comment
serhiy-storchaka left a comment There was a problem hiding this comment.
LGTM. In the current paradigm it is the right solution.
I wonder whether it would be better to perform this check in doRollover(), before renaming files. But this is a different and not so easy issue.
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.) |
serhiy-storchaka added needs backport to 3.12 
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 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. (cherry picked from commit e9b4ec6) 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 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. (cherry picked from commit e9b4ec6) 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 the 3.12 branch. |
GH-121117 is a backport of this pull request to the 3.13 branch. |
serhiy-storchaka commented Jun 28, 2024
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 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. (cherry picked from commit e9b4ec6) 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 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. (cherry picked from commit e9b4ec6) 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 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. 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 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. 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 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. Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
…andlers This fixes regression introduced in pythonGH-105887.
…andlers (pythonGH-143259) This fixes regression introduced in pythonGH-105887. (cherry picked from commit aa8a43d) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
…andlers (pythonGH-143259) This fixes regression introduced in pythonGH-105887. (cherry picked from commit aa8a43d) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
7 participants
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.