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

bpo-46712: Share global string identifiers in deepfreeze #31261

Merged
merged 3 commits into from
Feb 25, 2022

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Feb 10, 2022

Since bpo-46541, the global strings are statically allocated so they can now be referenced by deep-frozen modules just like any other singleton. Sharing identifiers with deepfreeze will reduce the duplicated strings hence it would save space.

See faster-cpython/ideas#218
See faster-cpython/ideas#230

cat Python/deepfreeze/deepfreeze.c | grep "_Py_ID" | wc -l 
1850

https://bugs.python.org/issue46712

@ericsnowcurrently
Copy link
Member

Extrapolating the list of global strings dynamically is a good idea. It would be best to do it in a separate PR. I've done so in gh-31346.

ericsnowcurrently added a commit that referenced this pull request Feb 15, 2022
Instead of manually enumerating the global strings in generate_global_objects.py, we extrapolate the list from usage of _Py_ID() and _Py_STR() in the source files.

This is partly inspired by gh-31261.

https://bugs.python.org/issue46541
@gvanrossum
Copy link
Member

@kumaraditya303 Could you fix the conflicts?

@kumaraditya303 kumaraditya303 marked this pull request as ready for review February 24, 2022 07:52
@kumaraditya303
Copy link
Contributor Author

@kumaraditya303 Could you fix the conflicts?

Fixed

@kumaraditya303
Copy link
Contributor Author

@gvanrossum This is ready for review.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LGTM.

I looked at some of the generated output and this feels like it's a good improvement.

Tools/scripts/deepfreeze.py Show resolved Hide resolved
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Thanks. Waiting for the tests to run once more.

(Also, please go fix the interning error handling, even if you disagree.)

@kumaraditya303
Copy link
Contributor Author

(Also, please go fix the interning error handling, even if you disagree.)

Okay, will create a PR tomorrow.

@gvanrossum gvanrossum merged commit eb002db into python:main Feb 25, 2022
@gvanrossum
Copy link
Member

Thanks Kumar! It's nice to see us regain some of the space costs of deep-freezing modules.

@kumaraditya303 kumaraditya303 deleted the global-strings branch February 25, 2022 18:33
asvetlov pushed a commit that referenced this pull request Feb 26, 2022
Where appropriate, deepfreeze.c now uses `&_Py_ID(blah)` references instead of locally defining constants. This saves some space.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants