bpo-1635741: Clean sysdict and builtins of interpreter#21605
bpo-1635741: Clean sysdict and builtins of interpreter#21605vstinner merged 5 commits intopython:masterfrom
Conversation
|
The master benchmark: After this PR: |
|
@vstinner Hi, victor, pls take a look after your vacation, thanks. |
|
thanks Dong-hee Na for your label ;) |
Python/pystate.c
Outdated
| /* We don't clear sysdict and builtins until the end of this function. | ||
| Because clearing other attributes can execute arbitrary Python code | ||
| which reuqires sysdict and builtins. */ | ||
| PyDict_Clear(sysdict); |
There was a problem hiding this comment.
I don't see the point of adding two local variables, why not accessing directly the structure member?
| PyDict_Clear(sysdict); | |
| PyDict_Clear(interp->sysdict); |
Same remark on folowing line.
There was a problem hiding this comment.
interp->sysdict and interp->builtins will be cleared before this line. https://github.com/python/cpython/blob/master/Python/pystate.c#L297-L298
There was a problem hiding this comment.
Aha, I didn't notice. In this case, I suggest to move Py_CLEAR(interp->sysdict) and Py_CLEAR(interp->builtins) at the end. Something like:
PyDict_Clear(interp->sysdict);
PyDict_Clear(interp->builtins);
Py_CLEAR(interp->sysdict)
Py_CLEAR(interp->builtins);
There was a problem hiding this comment.
updated. And I delete the description info.
What is this benchmark? Which code did you run? |
Hm, This should not be called as benchmark. I compare the refleak of this PR with the refleak of master branch. I use debug mode to test it. Because the testcase in https://bugs.python.org/issue1635741#msg355187 use |
|
Since few people care about subinterpreters, I don't think that it's worth it to document this internal change in a NEWS entry: I added "skip news" label. |
|
thanks, victor. |
https://bugs.python.org/issue1635741