classification
Title: opcode cache for LOAD_GLOBAL emits false alarm in memory leak hunting
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: christian.heimes Nosy List: gvanrossum, methane, pablogsal, vstinner, webstarnantes, yselivanov
Priority: normal Keywords: patch

Created on 2019-06-03 20:34 by vstinner, last changed 2021-03-08 21:56 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 13787 merged methane, 2019-06-03 22:19
PR 13789 closed pablogsal, 2019-06-03 22:27
PR 24582 vstinner, 2021-02-19 16:34
PR 24643 merged pablogsal, 2021-02-24 22:39
PR 24786 merged vstinner, 2021-03-08 20:04
Messages (13)
msg344469 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-03 20:34
opcode cache for LOAD_GLOBAL introduced false alarm in memory leak hunting (python3 -m test -R 3:3 ...).

=> opcache: bpo-26219.


Before the change:

$ git checkout 91234a16367b56ca03ee289f7c03a34d4cfec4c8^
$ make && ./python -m test -R 3:3 test_pprint 
...
Tests result: SUCCESS

After the change:

$ git checkout 91234a16367b56ca03ee289f7c03a34d4cfec4c8
$ make && ./python -m test -R 3:3 test_pprint 
...
test_pprint leaked [4, 2, 4] memory blocks, sum=10
...

The problem is that at each iteration of regrtest -R 3:3 (6 iterations), a few more code objects get this opcache allocated.

There are different solutions to fix regrtest -R 3:3.


(*) Always optimize:

diff --git a/Python/ceval.c b/Python/ceval.c
index 411ba3d73c..6cd148efba 100644
--- a/Python/ceval.c
+++ b/Python/ceval.c
@@ -103,7 +103,7 @@ static long dxp[256];
 #endif
 
 /* per opcode cache */
-#define OPCACHE_MIN_RUNS 1024  /* create opcache when code executed this time */
+#define OPCACHE_MIN_RUNS 1  /* create opcache when code executed this time */
 #define OPCACHE_STATS 0  /* Enable stats */
 
 #if OPCACHE_STATS

$ make && ./python -m test -R 3:3 test_pprint 
...
Tests result: SUCCESS


(*) Never optimmize: disable opcache until a better fix can be found

diff --git a/Python/ceval.c b/Python/ceval.c
index 411ba3d73c..3c85df6fea 100644
--- a/Python/ceval.c
+++ b/Python/ceval.c
@@ -1230,6 +1230,7 @@ _PyEval_EvalFrameDefault(PyFrameObject *f, int throwflag)
     f->f_stacktop = NULL;       /* remains NULL unless yield suspends frame */
     f->f_executing = 1;
 
+#if 0
     if (co->co_opcache_flag < OPCACHE_MIN_RUNS) {
         co->co_opcache_flag++;
         if (co->co_opcache_flag == OPCACHE_MIN_RUNS) {
@@ -1244,6 +1245,7 @@ _PyEval_EvalFrameDefault(PyFrameObject *f, int throwflag)
 #endif
         }
     }
+#endif
 
 #ifdef LLTRACE
     lltrace = _PyDict_GetItemId(f->f_globals, &PyId___ltrace__) != NULL;

$ make && ./python -m test -R 3:3 test_pprint 
...
Tests result: SUCCESS


(*) Find a way to explicitly deoptimize all code objects

Modules/gcmodule.c has a clear_freelists() function called by collect() if generation == NUM_GENERATIONS-1: on when gc.collect() is collected explicitly for example.

Lib/test/libregrtest/refleak.py also has a dash_R_cleanup() function which clears many caches.

Problem: currently, code objects are not explicitly tracked (for example, they are not tracked in a double linked list).


(*) Add way more warmup iterations to regrtest in buildbots.

I dislike this option. A build on a refleak buildbot worker already takes 2 to 3 hours. Adding more warmup would make a build even way more slower.
msg344485 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-03 22:38
New changeset eddef861b49f1635222a9e1771231c34a807debf by Victor Stinner (Inada Naoki) in branch 'master':
bpo-37146: disable opcache when Py_DEBUG is defined (GH-13787)
https://github.com/python/cpython/commit/eddef861b49f1635222a9e1771231c34a807debf
msg344486 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-03 22:41
INADA-san wrote PR 13787 to disable opcache when Python is compiled in debug mode. Pablo and me approved the change, so I merged it.

Pablo wrote a more robust solution, PR 13789, to disable opcache only in regrtest, to look for memory leaks. But INADA-san had a good argument against this approach:
https://github.com/python/cpython/pull/13789#issuecomment-498449735

"The code object will be optimized only when ++co->co_opcache_flag == opcacheminruns. So decreasing min_runs by _setopcacheminruns()will cause some hot codes will be not optimized forever. I don't want to expose such switch."

I would prefer to keep this issue until a long term approach is designed.
msg353666 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-01 10:50
It seems like nobody came up with a solution for the debug mode since June. I close the issue. Reopen it if you would like to propose a solution.
msg353700 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2019-10-01 16:02
> It seems like nobody came up with a solution for the debug mode since June. I close the issue. Reopen it if you would like to propose a solution.

I think the only solution is to have a flag to disable optimizations, inlcluding this one.
msg387335 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-02-19 17:35
Please revert this and use a separate build flag (e.g. DISABLE_INLINE_CACHES) for the refleaks run. (Or perhaps provide an -X flag to disable it without the need to recompile.)

I am developing new inline cache ideas and of course I need to run in debug mode, so I regularly end up wasting some time until I remember to add a patch that changes OPCACHE_MIN_RUNS back.
msg387338 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-02-19 17:49
I reopen the issue (but I'm not interested by trying to fix it).
msg387339 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-02-19 17:52
> (Or perhaps provide an -X flag to disable it without the need to recompile.)

Giving the ability to control the cache size, at least at Python startup, is one option.

Or maybe need a generic -X flag to tell Python that libregrtest is tracking leaks, since we might need to change other parameters than only the opcode cache size.
msg387361 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2021-02-19 22:53
> Giving the ability to control the cache size, at least at Python startup, is one option.

I'd really prefer not to allow users to control cache sizes. There's basically no point in that; the only practically useful thing is to enable or disable it.
msg387368 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-02-19 23:38
> Or maybe need a generic -X flag to tell Python that libregrtest is tracking leaks, since we might need to change other parameters than only the opcode cache size.

I think we should go this way, maybe even less "public". If we add a -X, then it will be too visible. Maybe some env variable?
msg387400 - (view) Author: Webstar Website (webstarnantes) Date: 2021-02-20 09:54
dans cette version pour securité maximale 

SSLContext.wrap_socket()
import socket
import ssl

hostname = 'www.python.org'
context = ssl.create_default_context()

with socket.create_connection((hostname, 443)) as sock:
    with context.wrap_socket(sock, server_hostname=hostname) as ssock:
        print(ssock.version())


https://backlinkstrong.com/
msg387820 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-02-28 22:41
New changeset af5fa13ef6f648fc7a7a33a7556db13887e7d643 by Pablo Galindo in branch 'master':
bpo-37146: Deactivate opcode cache only when using huntrleaks in the test suite (GH-24643)
https://github.com/python/cpython/commit/af5fa13ef6f648fc7a7a33a7556db13887e7d643
msg388311 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-03-08 21:56
New changeset 9f672a52f55ce198d5689b5771948350028b4a3b by Victor Stinner in branch 'master':
bpo-37146: Move _PyEval_DeactivateOpCache() to the internal C API (GH-24786)
https://github.com/python/cpython/commit/9f672a52f55ce198d5689b5771948350028b4a3b
History
Date User Action Args
2021-03-08 21:56:41vstinnersetmessages: + msg388311
2021-03-08 20:04:04vstinnersetpull_requests: + pull_request23552
2021-02-28 22:42:42pablogsalsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2021-02-28 22:41:15pablogsalsetmessages: + msg387820
2021-02-24 22:39:58pablogsalsetstage: resolved -> patch review
pull_requests: + pull_request23428
2021-02-20 10:26:31christian.heimessetnosy: - christian.heimes

type: security -> behavior
components: + Interpreter Core, - SSL
title: SSl Securité version 3.9.2 -> opcode cache for LOAD_GLOBAL emits false alarm in memory leak hunting
2021-02-20 09:54:30webstarnantessetassignee: christian.heimes
type: security

components: + SSL, - Interpreter Core
versions: + Python 3.9, - Python 3.8
nosy: + christian.heimes, webstarnantes
title: opcode cache for LOAD_GLOBAL emits false alarm in memory leak hunting -> SSl Securité version 3.9.2
messages: + msg387400
2021-02-19 23:38:02pablogsalsetnosy: + pablogsal
messages: + msg387368
2021-02-19 22:53:54yselivanovsetmessages: + msg387361
2021-02-19 17:52:54vstinnersetmessages: + msg387339
2021-02-19 17:49:28vstinnersetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg387338
2021-02-19 17:35:36gvanrossumsetnosy: + gvanrossum
messages: + msg387335
2021-02-19 16:34:45vstinnersetpull_requests: + pull_request23365
2019-10-01 16:02:56yselivanovsetmessages: + msg353700
2019-10-01 10:50:13vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg353666

stage: patch review -> resolved
2019-06-03 22:41:07vstinnersetmessages: + msg344486
2019-06-03 22:38:13vstinnersetmessages: + msg344485
2019-06-03 22:27:26pablogsalsetpull_requests: + pull_request13673
2019-06-03 22:19:57methanesetkeywords: + patch
stage: patch review
pull_requests: + pull_request13671
2019-06-03 20:34:43vstinnercreate