Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
bpo-1635741: Clean sysdict and builtins of interpreter#21605
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Conversation
shihai1991 commented Jul 24, 2020 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
shihai1991 commented Jul 24, 2020
The master benchmark: After this PR: |
shihai1991 commented Jul 24, 2020
@vstinner Hi, victor, pls take a look after your vacation, thanks. |
bedevere-bot commented Jul 27, 2020
shihai1991 commented Jul 27, 2020
thanks Dong-hee Na for your label ;) |
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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); There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
vstinner commented Aug 5, 2020
What is this benchmark? Which code did you run? |
shihai1991 commented Aug 5, 2020
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 |
Uh oh!
There was an error while loading. Please reload this page.
vstinner commented Aug 6, 2020
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. |
vstinner left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
shihai1991 commented Aug 13, 2020
thanks, victor. |
https://bugs.python.org/issue1635741