bpo-1635741: Clean sysdict and builtins of interpreter by shihai1991 · Pull Request #21605 · python/cpython
The master benchmark:
sys.gettotalrefcount: 14779
sys.gettotalrefcount: 19024
sys.gettotalrefcount: 23269
sys.gettotalrefcount: 27514
sys.gettotalrefcount: 31759
sys.gettotalrefcount: 36004
sys.gettotalrefcount: 40249
sys.gettotalrefcount: 44494
sys.gettotalrefcount: 48739
sys.gettotalrefcount: 52984
After this PR:
sys.gettotalrefcount: 14295
sys.gettotalrefcount: 18056
sys.gettotalrefcount: 21817
sys.gettotalrefcount: 25578
sys.gettotalrefcount: 29339
sys.gettotalrefcount: 33100
sys.gettotalrefcount: 36861
sys.gettotalrefcount: 40622
sys.gettotalrefcount: 44383
sys.gettotalrefcount: 48144
@vstinner Hi, victor, pls take a look after your vacation, thanks.
thanks Dong-hee Na for your label ;)
| /* 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); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated. And I delete the description info.
The master benchmark:
What is this benchmark? Which code did you run?
The master benchmark:
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 _Py_GetRefTotal()
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
