GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||
GeezerVison is showing github.com — gh-105623 Fix performance degradation in logging RotatingFileHandler by zhatt · Pull Request #105887 · python/cpython Conversation
|
|
🤖 New build scheduled with the buildbot fleet by @vsajip for commit 84ad7d8 🤖 If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| # 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.
The reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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 in doRollover(), before renaming files. But this is a different and not so easy issue.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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.) |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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.13. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…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 <[email protected]> Co-authored-by: Oleg Iarygin <[email protected]>
…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 <[email protected]> Co-authored-by: Oleg Iarygin <[email protected]>
|
GH-121116 is a backport of this pull request to the 3.12 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
GH-121117 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thank you for your contribution @zhatt, and thank you for reminder @fantabolous. I forgot about this PR. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…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 <[email protected]> Co-authored-by: Oleg Iarygin <[email protected]>
…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 <[email protected]> Co-authored-by: Oleg Iarygin <[email protected]>
…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 <[email protected]>
…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 <[email protected]>
…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 <[email protected]>
…andlers (pythonGH-143259) This fixes regression introduced in pythonGH-105887. (cherry picked from commit aa8a43d) Co-authored-by: Serhiy Storchaka <[email protected]>
…andlers (pythonGH-143259) This fixes regression introduced in pythonGH-105887.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.