GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-105623 Fix performance degradation in logging RotatingFileHandler by zhatt · Pull Request #105887 · python/cpython

Conversation

@zhatt

@zhatt zhatt commented Jun 17, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

  • Issue: Poor performance on logging.RotatingFileHandler due to fix to #89564 #105623

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

arhadthedev and bdraco reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@zhatt
pythongh-105623 Fix performance degradation in logging RotatingFileHa…
d3a1875
…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.
@zhatt
zhatt requested a review from vsajip as a code owner June 17, 2023 16:17
@bedevere-bot bedevere-bot added the awaiting review label Jun 17, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jun 17, 2023
Poor performance on logging.RotatingFileHandler due to fix to #89564 #105623
Closed
@ghost

ghost commented Jun 17, 2023 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@zhatt
Merge branch 'main' into pythongh-105623
b1d5ba1
@arhadthedev arhadthedev added the performance Performance or resource usage label Jun 17, 2023
arhadthedev
arhadthedev reviewed Jun 17, 2023
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2023-06-17-09-07-06.gh-issue-105623.5G06od.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@zhatt @arhadthedev
Update Misc/NEWS.d/next/Library/2023-06-17-09-07-06.gh-issue-105623.5…
84ad7d8
…G06od.rst

Co-authored-by: Oleg Iarygin <[email protected]>
@vsajip vsajip added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 19, 2023
@bedevere-bot

bedevere-bot commented Jun 19, 2023

Copy link
Copy Markdown

🤖 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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 19, 2023
@arhadthedev

arhadthedev commented Jun 19, 2023

Copy link
Copy Markdown
Member

From buildbots:

Objects/object.c:2197: _Py_ForgetReference: Assertion failed: invalid object chain
Enable tracemalloc to get the memory block allocation traceback
object address  : 0x7ff344737770
object refcount : 0
object type     : 0x560543494660
object type name: tuple
object repr     : <refcnt 0 at 0x7ff344737770>

This PR is not the cause (Python-only change cannot break reference counting). However, we need to fix the crash before merging the PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@zhatt
Merge branch 'main' into pythongh-105623
a677f11
bdraco
bdraco reviewed Jul 13, 2023
View reviewed changes
Comment thread Lib/logging/handlers.py
Comment on lines -190 to -198
# 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

@bdraco bdraco Jul 13, 2023

Copy link
Copy Markdown
Contributor

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

Screenshot 2023-07-12 at 6 22 51 PM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@zhatt zhatt Jul 13, 2023

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@bdraco bdraco Jul 13, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@vsajip vsajip Jul 13, 2023

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

methane reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@zhatt
Merge branch 'main' into pythongh-105623
e3ee1d7
serhiy-storchaka
serhiy-storchaka approved these changes Mar 3, 2024
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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 in doRollover(), before renaming files. But this is a different and not so easy issue.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bdraco reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Mar 3, 2024
@serhiy-storchaka
Merge branch 'main' into pythongh-105623
3ff1523
@fantabolous

fantabolous commented Jun 27, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@zhatt
Merge branch 'main' into pythongh-105623
7b5189b
@serhiy-storchaka
serhiy-storchaka enabled auto-merge (squash) June 27, 2024 16:36
@serhiy-storchaka
serhiy-storchaka merged commit e9b4ec6 into python:main Jun 27, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Jun 27, 2024
@serhiy-storchaka serhiy-storchaka added needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes labels Jun 28, 2024
@miss-islington-app

miss-islington-app Bot commented Jun 28, 2024

Copy link
Copy Markdown

Thanks @zhatt for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington-app

miss-islington-app Bot commented Jun 28, 2024

Copy link
Copy Markdown

Thanks @zhatt for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 28, 2024
@zhatt @arhadthedev @miss-islington
pythongh-105623 Fix performance degradation in logging RotatingFileHa…
5b73762
…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]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 28, 2024
@zhatt @arhadthedev @miss-islington
pythongh-105623 Fix performance degradation in logging RotatingFileHa…
6e494b3
…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]>
@bedevere-app

bedevere-app Bot commented Jun 28, 2024

Copy link
Copy Markdown

GH-121116 is a backport of this pull request to the 3.12 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Jun 28, 2024
@bedevere-app

bedevere-app Bot commented Jun 28, 2024

Copy link
Copy Markdown

GH-121117 is a backport of this pull request to the 3.13 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Jun 28, 2024
@serhiy-storchaka

serhiy-storchaka commented Jun 28, 2024

Copy link
Copy Markdown
Member

Thank you for your contribution @zhatt, and thank you for reminder @fantabolous. I forgot about this PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka pushed a commit that referenced this pull request Jun 28, 2024
@miss-islington @zhatt @arhadthedev
[3.12] gh-105623 Fix performance degradation in logging RotatingFileH…
2a68ed9
…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]>
serhiy-storchaka pushed a commit that referenced this pull request Jun 28, 2024
@miss-islington @zhatt @arhadthedev
[3.13] gh-105623 Fix performance degradation in logging RotatingFileH…
e310d2b
…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]>
mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
@zhatt @arhadthedev @mrahtz
pythongh-105623 Fix performance degradation in logging RotatingFileHa…
48f686e
…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]>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@zhatt @arhadthedev @estyxx
pythongh-105623 Fix performance degradation in logging RotatingFileHa…
467286e
…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]>
@rubiksdot rubiksdot mentioned this pull request Dec 28, 2025
logging.handler fails when attempting to rotate a fifo (with potential fix) #143237
Closed
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Dec 28, 2025
@serhiy-storchaka
pythongh-143237: Fix support of named pipes in the rotating logging h…
cc46702
…andlers

This fixes regression introduced in pythonGH-105887.
@serhiy-storchaka serhiy-storchaka mentioned this pull request Dec 28, 2025
gh-143237: Fix support of named pipes in the rotating logging handlers #143259
Merged
serhiy-storchaka added a commit that referenced this pull request Dec 30, 2025
@serhiy-storchaka
gh-143237: Fix support of named pipes in the rotating logging handlers (
aa8a43d
GH-143259)

This fixes regression introduced in GH-105887.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 30, 2025
@serhiy-storchaka @miss-islington
pythongh-143237: Fix support of named pipes in the rotating logging h…
f45b111
…andlers (pythonGH-143259)

This fixes regression introduced in pythonGH-105887.
(cherry picked from commit aa8a43d)

Co-authored-by: Serhiy Storchaka <[email protected]>
@miss-islington miss-islington mentioned this pull request Dec 30, 2025
[3.14] gh-143237: Fix support of named pipes in the rotating logging handlers (GH-143259) #143297
Merged
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 30, 2025
@serhiy-storchaka @miss-islington
pythongh-143237: Fix support of named pipes in the rotating logging h…
6106e1f
…andlers (pythonGH-143259)

This fixes regression introduced in pythonGH-105887.
(cherry picked from commit aa8a43d)

Co-authored-by: Serhiy Storchaka <[email protected]>
@miss-islington miss-islington mentioned this pull request Dec 30, 2025
[3.13] gh-143237: Fix support of named pipes in the rotating logging handlers (GH-143259) #143298
Merged
pablogsal pushed a commit that referenced this pull request Jan 25, 2026
@miss-islington @serhiy-storchaka
[3.13] gh-143237: Fix support of named pipes in the rotating logging …
8ad8287
…handlers (GH-143259) (#143298)

gh-143237: Fix support of named pipes in the rotating logging handlers (GH-143259)

This fixes regression introduced in GH-105887.
(cherry picked from commit aa8a43d)

Co-authored-by: Serhiy Storchaka <[email protected]>
thunder-coding pushed a commit to thunder-coding/cpython that referenced this pull request Feb 15, 2026
@serhiy-storchaka @thunder-coding
pythongh-143237: Fix support of named pipes in the rotating logging h…
8add574
…andlers (pythonGH-143259)

This fixes regression introduced in pythonGH-105887.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arhadthedev arhadthedev arhadthedev left review comments
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@vsajip vsajip Awaiting requested review from vsajip vsajip is a code owner
+1 more reviewer
@bdraco bdraco bdraco left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

performance Performance or resource usage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@zhatt @bedevere-bot @arhadthedev @fantabolous @serhiy-storchaka @vsajip @bdraco
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.