bpo-27535: Fix memory leak with warnings ignore by vstinner · Pull Request #4489 · python/cpython

vstinner · 2017-11-21T15:09:13Z

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

I want to play with the code more.

serhiy-storchaka · 2017-11-22T18:14:36Z

Python/_warnings.c

-        else if (_PyUnicode_EqualToASCIIString(action, "ignore"))
+        }
+
+        if (_PyUnicode_EqualToASCIIString(action, "ignore"))


Isn't this already tested above?

I removed it. I kept it for tests, but then I forgot to remove it.

serhiy-storchaka · 2017-11-22T18:16:13Z

Python/_warnings.c

    if (!_PyUnicode_EqualToASCIIString(action, "always")) {
        if (registry != NULL && registry != Py_None &&
-                PyDict_SetItem(registry, key, Py_True) < 0)
+                PyDict_SetItem(registry, key, Py_True) < 0) {


If you want to add braces, place the opening brace after a multiline condition on a separate line.

The PEP 7 only shows "if (test) {", it says nothing about multiline condition. Until a newline is required by the PEP 7, I prefer to put "{" on the same line.

* When you break a long expression at a binary operator, the operator goes at the end of the previous line, and braces should be formatted as shown. E.g.:: if (type->tp_dictoffset != 0 && base->tp_dictoffset == 0 && type->tp_dictoffset == b_size && (size_t)t_size == b_size + sizeof(PyObject *)) { return 0; /* "Forgive" adding a __dict__ only */ }

Oh nice, it's specified in the PEP 7. In that case, ok to reformat the code as the PEP 7 requires ;-)

serhiy-storchaka · 2017-11-22T18:23:30Z

Lib/warnings.py

-            re.compile(module), lineno, append=append)
+
+    if message:
+        regex = re.compile(message, re.I)


This could be written shorter.

regex = re.compile(message, re.I) if message else None module = re.compile(module) if module else None _add_filter(action, regex, category, module, lineno, append=append)

serhiy-storchaka · 2017-11-22T18:26:06Z

Lib/test/test_warnings/__init__.py

            self.module.filterwarnings("ignore", category=UserWarning)
            self.module.warn("FilterTests.test_ignore", UserWarning)
            self.assertEqual(len(w), 0)
+            self.assertEqual(list(__warningregistry__.keys()),


This line doesn't look too long for splitting it. Especially if remove redundant .keys().

vstinner · 2017-11-23T00:40:19Z

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.

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 · 2017-11-23T09:41:35Z

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

vstinner · 2017-11-23T16:39:55Z

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 · 2017-11-23T17:19:33Z

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

vstinner · 2017-11-23T17:37:28Z

@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 · 2017-11-23T17:43:41Z

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 · 2017-11-27T15:55:44Z

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

miss-islington · 2017-11-27T16:04:19Z

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

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 · 2017-11-27T16:04:30Z

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

vstinner · 2017-11-27T16:08:46Z

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

vstinner requested a review from serhiy-storchaka November 21, 2017 15:09

the-knights-who-say-ni added the CLA signed label Nov 21, 2017

bedevere-bot added the awaiting merge label Nov 21, 2017

serhiy-storchaka reviewed Nov 22, 2017

View reviewed changes

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.

serhiy-storchaka approved these changes Nov 23, 2017

View reviewed changes

vstinner merged commit c975878 into python:master Nov 27, 2017

bedevere-bot removed the awaiting merge label Nov 27, 2017

vstinner deleted the warn_ignore branch November 27, 2017 15:57

vstinner added the needs backport to 3.6 label Nov 27, 2017

bedevere-bot removed the needs backport to 3.6 label Nov 27, 2017

ghost mentioned this pull request Sep 12, 2022

Ignored ResourceWarning warnings leak memory in warnings registries #71722

Closed

vstinner mentioned this pull request Sep 26, 2022

Memory leak: warnings module #97557

Open

Uh oh!

Conversation

vstinner commented Nov 21, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 23, 2017

Uh oh!

vstinner commented Nov 23, 2017

Uh oh!

vstinner commented Nov 23, 2017

Uh oh!

serhiy-storchaka commented Nov 23, 2017

Uh oh!

vstinner commented Nov 23, 2017

Uh oh!

serhiy-storchaka commented Nov 23, 2017

Uh oh!

vstinner commented Nov 27, 2017

Uh oh!

miss-islington commented Nov 27, 2017

Uh oh!

bedevere-bot commented Nov 27, 2017

Uh oh!

vstinner commented Nov 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vstinner commented Nov 21, 2017 •

edited by bedevere-bot

Loading