◐ Shell
reader mode source ↗
Skip to content

bpo-27535: Fix memory leak with warnings ignore#4489

Merged
vstinner merged 1 commit into
python:masterfrom
vstinner:warn_ignore
Nov 27, 2017
Merged

bpo-27535: Fix memory leak with warnings ignore#4489
vstinner merged 1 commit into
python:masterfrom
vstinner:warn_ignore

Conversation

@vstinner

@vstinner vstinner commented Nov 21, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

The warnings module doesn't leak memory anymore in the hidden
warnings registry for the "ignore" action of warnings filters.

The warn_explicit() function doesn't add the warning key to the
registry anymore for the "ignore" action.

https://bugs.python.org/issue27535

serhiy-storchaka
serhiy-storchaka reviewed Nov 22, 2017
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

I want to play with the code more.

@vstinner

vstinner commented Nov 23, 2017

Copy link
Copy Markdown
Member Author

Ooops, previously, I pushed optimization commits in this PR by mistake. It's now fixed. Again, this PR is only the change to leave the registry unchanged for the "ignore" action.

@vstinner
bpo-27535: Fix memory leak with warnings ignore …
c6c46a3 
The warnings module doesn't leak memory anymore in the hidden
warnings registry for the "ignore" action of warnings filters.

The warn_explicit() function doesn't add the warning key to the
registry anymore for the "ignore" action.
@vstinner

vstinner commented Nov 23, 2017

Copy link
Copy Markdown
Member Author

I reformatted the if with multiline condition and rebased my PR.

@vstinner

vstinner commented Nov 23, 2017

Copy link
Copy Markdown
Member Author

Latest reliable (using isolated CPUs) benchmark:

vstinner@apu$ ./python -m perf compare_to master.json ignore.json 
Mean +- std dev: [master] 705 ns +- 24 ns -> [ignore] 838 ns +- 18 ns: 1.19x slower (+19%)

So this PR adds a slowdown of +133 ns on warning.warn() when the warning is ignored.

This slowdown can be removed using a "C cache" of warnings.filters: the optimization implemented in PR #4502. The problem is that I'm not sure that it's ok to cache warnings.filters and/or make warnings.filters "immutable" (convert it to a tuple).

@serhiy-storchaka

serhiy-storchaka commented Nov 23, 2017

Copy link
Copy Markdown
Member

The overhead depends on the number and kind of filters. For warnings emitted in a C code the relative slowdown will be larger.

@vstinner

vstinner commented Nov 23, 2017

Copy link
Copy Markdown
Member Author

@serhiy-storchaka: Are you ok to merge this PR even with the "1.2x slower" (+19%)? Or do you prefer to wait until a decision can be made on a "C cache" for warnings.filters?

@serhiy-storchaka

serhiy-storchaka commented Nov 23, 2017

Copy link
Copy Markdown
Member

Could you please make a benchmark for warnings emitted in a tight loop in the C code. It would be nice to know what is the worst case. If it is less than 2x slower I will prefer this simpler PR.

@vstinner

vstinner commented Nov 27, 2017

Copy link
Copy Markdown
Member Author

Could you please make a benchmark for warnings emitted in a tight loop in the C code. It would be nice to know what is the worst case. If it is less than 2x slower I will prefer this simpler PR.

It's not 2x slower, it's between 1.03x slower (+21 ns) and 1.09x slower (+79 ns):
https://bugs.python.org/issue27535#msg306919

@vstinner vstinner merged commit c975878 into python:master Nov 27, 2017
@vstinner vstinner deleted the warn_ignore branch November 27, 2017 15:57
@miss-islington

miss-islington commented Nov 27, 2017

Copy link
Copy Markdown
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 27, 2017
@vstinner @miss-islington
bpo-27535: Fix memory leak with warnings ignore (pythonGH-4489) …
23c01ce 
The warnings module doesn't leak memory anymore in the hidden
warnings registry for the "ignore" action of warnings filters.

The warn_explicit() function doesn't add the warning key to the
registry anymore for the "ignore" action.
(cherry picked from commit c975878)
@bedevere-bot

bedevere-bot commented Nov 27, 2017

Copy link
Copy Markdown

GH-4587 is a backport of this pull request to the 3.6 branch.

@vstinner

vstinner commented Nov 27, 2017

Copy link
Copy Markdown
Member Author

I backported manually the change to Python 2.7: PR #4588.

@ghost ghost mentioned this pull request Sep 12, 2022
Closed
@vstinner vstinner mentioned this pull request Sep 26, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vstinner @serhiy-storchaka @miss-islington @bedevere-bot @the-knights-who-say-ni