Skip to content
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

GH-90699: Intern statically allocated strings #93597

Merged
merged 15 commits into from
Jul 8, 2022

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Jun 8, 2022

This is similar to how strings are interned for deepfreeze.

Automerge-Triggered-By: GH:ericsnowcurrently

@kumaraditya303 kumaraditya303 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 8, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit f0e01c4 🤖

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 8, 2022
@kumaraditya303 kumaraditya303 marked this pull request as ready for review June 8, 2022 13:22
@AA-Turner
Copy link
Member

Should pycore_runtime_init.h be marked as generated?

cpython/.gitattributes

Lines 69 to 71 in 243ed54

Include/internal/pycore_ast.h generated
Include/internal/pycore_ast_state.h generated
Include/internal/pycore_opcode.h generated

A

@kumaraditya303
Copy link
Contributor Author

Should pycore_runtime_init.h be marked as generated?

That file also contains handwritten struct initialization macros so I don't think so.

@ericsnowcurrently
Copy link
Member

That file also contains handwritten struct initialization macros so I don't think so.

Yeah, only _Py_global_objects_INIT is generated and we want changes to the rest to get expanded by default in PRs.

I suppose we could separate the generated part from the rest, AKA pycore_global_objects_init.h (or even _pycore_global_objects_init.h). I doubt that would be a problem, especially if it makes occasional PRs a little easier to review (e.g. no ambiguity about what is generated and what itsn't).

@ericsnowcurrently
Copy link
Member

Would we also move the generated parts of pycore_global_strings.h to a separate header file (for the same reasons)?

@AA-Turner
Copy link
Member

With this PR, ~93% of the lines in pycore_runtime_init.h are autogenerated -- I did miss that it starts with ~120 lines of handwritten code, but for future maintenence / ease of review I'd agree with Eric that it might be better to move to its own file. (A large part is also my unfamiliarity with this part of the code, to be fair -- if it causes more hassle than it's worth then the status quo wins).

A

@kumaraditya303
Copy link
Contributor Author

I can work on a PR to separate the generated and handwritten code but it will be a separate PR. I have other changes enqueued for static strings which depend on this PR so I want to get this in first.

@kumaraditya303
Copy link
Contributor Author

@ericsnowcurrently FYI, I have created #94051 to separate generated code.

@gvanrossum gvanrossum removed their request for review June 27, 2022 21:13
@gvanrossum
Copy link
Member

I'm hoping Eric can do the review here.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

This mostly looks good. I just have a few comments.

Tools/scripts/generate_global_objects.py Outdated Show resolved Hide resolved
Include/internal/pycore_runtime_init.h Outdated Show resolved Hide resolved
Include/internal/pycore_runtime_init.h Outdated Show resolved Hide resolved
Python/pylifecycle.c Outdated Show resolved Hide resolved
Include/internal/pycore_runtime_init.h Outdated Show resolved Hide resolved
Include/internal/pycore_runtime_init.h Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

Thanks for make those updates! This is just about ready. I'll approve and merge it once you've merged in the gh-94051 changes.

Tools/scripts/generate_global_objects.py Outdated Show resolved Hide resolved
Tools/scripts/generate_global_objects.py Show resolved Hide resolved
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

@ericsnowcurrently
Copy link
Member

Thanks for working on this, @kumaraditya303!

@kumaraditya303
Copy link
Contributor Author

Thanks for working on this

Thanks for the reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants