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 — gh-124785: re-work fix so tracerefs test passes by nascheme · Pull Request #124808 · python/cpython

Conversation

@nascheme

@nascheme nascheme commented Sep 30, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

The previous fix to this bug caused some trace-refs tests to fail. Object references from the sub-interpreters were not being correctly accounted in the main interpreter. Re-work the fix so that the interned strings for sub-interpreters go into their own dict, interned_strings_legacy. That allows the main interpreter to clean them knowing that those specific strings have been allocated in sub-interpreters.

  • Issue: [TraceRefs build] Tests fail: _PyRefchain_Remove: Assertion `value == REFCHAIN_VALUE' failed #124785

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nascheme
pythongh-124785: re-work fix so tracerefs test passes
d8bf7d9
@nascheme
nascheme requested a review from ericsnowcurrently as a code owner September 30, 2024 22:38
@bedevere-app bedevere-app Bot added the awaiting core review label Sep 30, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Sep 30, 2024
[TraceRefs build] Tests fail: _PyRefchain_Remove: Assertion `value == REFCHAIN_VALUE' failed #124785
Closed
@nascheme nascheme added the skip news label Sep 30, 2024
@nascheme
nascheme requested a review from vstinner September 30, 2024 22:39
@nascheme nascheme mentioned this pull request Sep 30, 2024
[3.13] gh-124785: alternative interned string clear fix #124796
Closed
ericsnowcurrently
ericsnowcurrently reviewed Sep 30, 2024
View reviewed changes
Comment thread Objects/unicodeobject.c
Comment on lines +304 to +305
return (interp != main_interp &&
!(interp->feature_flags & Py_RTFLAGS_MULTI_INTERP_EXTENSIONS));

@ericsnowcurrently ericsnowcurrently Sep 30, 2024

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.

FYI, (not Py_RTFLAGS_MULTI_INTERP_EXTENSIONS) does not imply Py_RTFLAGS_USE_MAIN_OBMALLOC. Why not stick with Py_RTFLAGS_USE_MAIN_OBMALLOC?

Sorry, something went wrong.

Uh oh!

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

@nascheme nascheme Sep 30, 2024

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.

I'm looking at reload_singlephase_extension and when we can end up in the PyDict_Update case. I think it can only happen if Py_RTFLAGS_MULTI_INTERP_EXTENSIONS is set. If we do that dict update then we are sharing objects between interpreters.

Sorry, something went wrong.

Uh oh!

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

Uh oh!

There was an error while loading. Please reload this page.
@nascheme @ericsnowcurrently
Update Objects/unicodeobject.c
fe6adde 
Spelling fix.

Co-authored-by: Eric Snow <[email protected]>
@nascheme

nascheme commented Oct 1, 2024

Copy link
Copy Markdown
Member Author

For the backport to 3.12 and 3.13 we could make interned_strings_legacy a global var rather than part of the interpreter state structure (if that's a problem for maintaining the ABI). I think that's safe because only the main interpreter would use that global.

That's assuming we think this change is appropriate as a backported fix. Another option would be GH-124796, simpler code but it leaks the immortal strings whereas this PR frees them (eventually).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Oct 1, 2024
View reviewed changes
Comment thread Objects/unicodeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/unicodeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@nascheme
Minor code cleanup as suggested by review.
1d0b7c1
@nascheme

nascheme commented Oct 1, 2024

Copy link
Copy Markdown
Member Author

Closing this. After discussion with Eric Snow, I think his GH-124865 is the preferred fix. It has the advantage that it's just a relatively small change on top of GH-124646 (to fix the tracerefs issue) and it doesn't touch the ABI like this one does. So, it should be easier to backport.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nascheme nascheme closed this Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@ericsnowcurrently ericsnowcurrently ericsnowcurrently left review comments

Uh oh!

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

Assignees

No one assigned

Labels

awaiting core review 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.

3 participants

@nascheme @vstinner @ericsnowcurrently
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.