Skip to content

bpo-46430: Intern strings in deep-frozen modules - #30683

Merged
gvanrossum merged 6 commits into
python:mainfrom
kumaraditya303:intern
Feb 9, 2022
Merged

bpo-46430: Intern strings in deep-frozen modules #30683
gvanrossum merged 6 commits into
python:mainfrom
kumaraditya303:intern

Conversation

@kumaraditya303

@kumaraditya303kumaraditya303 commented Jan 19, 2022•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

gvanrossum
gvanrossum reviewed Jan 19, 2022
View reviewed changes

@gvanrossumgvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling this isn't going to be enough, alas. :-(

I've tried to drop a few hints in the review. :-)

Comment threadTools/scripts/deepfreeze.py Outdated
Comment threadTools/scripts/deepfreeze.py Outdated
Comment threadTools/scripts/deepfreeze.py Outdated
@kumaraditya303

kumaraditya303 commented Jan 31, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

I'll revisit this once statically allocated strings land by @ericsnowcurrently See #30928

@kumaraditya303
kumaraditya303 marked this pull request as draft January 31, 2022 07:54
@gvanrossum

gvanrossum commented Jan 31, 2022

Copy link
Copy Markdown
Member

I'll revisit this once statically allocated strings land by @ericsnowcurrently

To be clear you're referencing faster-cpython/ideas#230 and #30928 here.

gvanrossum
gvanrossum approved these changes Feb 7, 2022
View reviewed changes

@gvanrossumgvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I just think we shouldn't ignore errors.

Comment threadObjects/codeobject.c
@kumaraditya303
kumaraditya303 marked this pull request as ready for review February 7, 2022 13:22
@kumaraditya303kumaraditya303 mentioned this pull request Feb 7, 2022
Closed
gvanrossum
gvanrossum approved these changes Feb 9, 2022
View reviewed changes

@gvanrossumgvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, fair enough.

@gvanrossum
gvanrossum merged commit c0a5ebe into python:mainFeb 9, 2022
@kumaraditya303
kumaraditya303 deleted the intern branch February 9, 2022 19:04
@vstinner

vstinner commented Feb 23, 2022

Copy link
Copy Markdown
Member

The change introduced a regression: https://bugs.python.org/issue46430#msg413822

@Christopher-ChianelliChristopher-Chianelli mentioned this pull request Apr 16, 2023
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@methanemethanemethane approved these changes
@gvanrossumgvanrossumgvanrossum approved these changes
@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@kumaraditya303@gvanrossum@vstinner@methane@the-knights-who-say-ni@bedevere-bot