GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-1635741: Clean sysdict and builtins of interpreter by shihai1991 · Pull Request #21605 · python/cpython

Conversation

@shihai1991

@shihai1991 shihai1991 commented Jul 24, 2020 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

https://bugs.python.org/issue1635741

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@shihai1991
Clean sysdict and builtins of interpreter.
137967a
@bedevere-bot bedevere-bot added the awaiting review label Jul 24, 2020
@shihai1991

shihai1991 commented Jul 24, 2020

Copy link
Copy Markdown
Member Author

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@shihai1991

shihai1991 commented Jul 24, 2020

Copy link
Copy Markdown
Member Author

@vstinner Hi, victor, pls take a look after your vacation, thanks.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10 corona10 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 27, 2020
@bedevere-bot

bedevere-bot commented Jul 27, 2020

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @corona10 for commit 137967a 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 27, 2020
@shihai1991

shihai1991 commented Jul 27, 2020

Copy link
Copy Markdown
Member Author

thanks Dong-hee Na for your label ;)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Aug 3, 2020
View reviewed changes
Comment thread Python/pystate.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@shihai1991
update code according victor's comment
2910570
vstinner
vstinner reviewed Aug 5, 2020
View reviewed changes
Comment thread 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);

@vstinner vstinner Aug 5, 2020

Copy link
Copy Markdown
Member

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?

Suggested change
PyDict_Clear(sysdict);
PyDict_Clear(interp->sysdict);

Same remark on folowing line.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@shihai1991 shihai1991 Aug 5, 2020

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Aug 5, 2020

Copy link
Copy Markdown
Member

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);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@shihai1991 shihai1991 Aug 5, 2020

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/pystate.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Aug 5, 2020

Copy link
Copy Markdown
Member

The master benchmark:

What is this benchmark? Which code did you run?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@shihai1991
Fix typo
ba98fc1
@shihai1991

shihai1991 commented Aug 5, 2020

Copy link
Copy Markdown
Member Author

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()
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@shihai1991
update code order
d08c18f
vstinner
vstinner reviewed Aug 6, 2020
View reviewed changes
Comment thread Python/pystate.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@shihai1991
update code order
a9df923
@vstinner vstinner added the skip news label Aug 6, 2020
@vstinner

vstinner commented Aug 6, 2020

Copy link
Copy Markdown
Member

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.
shihai1991 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner approved these changes Aug 6, 2020
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Aug 6, 2020
@vstinner
vstinner merged commit 8ecc0c4 into python:master Aug 12, 2020
@bedevere-bot bedevere-bot removed the awaiting merge label Aug 12, 2020
@shihai1991

shihai1991 commented Aug 13, 2020

Copy link
Copy Markdown
Member Author

thanks, victor.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
shihai1991 added a commit to shihai1991/cpython that referenced this pull request Aug 20, 2020
@shihai1991
bpo-1635741: Clean sysdict and builtins of interpreter at exit (pytho…
0b9ebc2 
…nGH-21605)
xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
@shihai1991 @xzy3-cdc
bpo-1635741: Clean sysdict and builtins of interpreter at exit (pytho…
2174691 
…nGH-21605)
@kylotan kylotan mannequin mentioned this pull request Sep 19, 2022
Py_Finalize() doesn't clear all Python objects at exit #44470
Closed
@methane methane mentioned this pull request Mar 21, 2024
Python 3.9 segfault in minimal tracing snippet open-telemetry/opentelemetry-python#3801
Closed
@methane methane mentioned this pull request Mar 21, 2024
Python <=3.9 segmentation fault in code snippet #117090
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@shihai1991 @bedevere-bot @vstinner @corona10
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.