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

ericsnowcurrently commented Feb 14, 2022

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

gvanrossum commented Feb 20, 2022

@kumaraditya303 Could you fix the conflicts?

@kumaraditya303 kumaraditya303 marked this pull request as ready for review Feb 24, 2022
@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Feb 24, 2022

@kumaraditya303 Could you fix the conflicts?

Fixed

@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Feb 25, 2022

@gvanrossum This is ready for review.

Copy link
Member

@gvanrossum gvanrossum left a comment

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

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

kumaraditya303 commented Feb 25, 2022

(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
11 of 12 checks passed
@gvanrossum
Copy link
Member

gvanrossum commented Feb 25, 2022

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 Feb 25, 2022
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.

None yet

5 participants