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 — bpo-46712: Share global string identifiers in deepfreeze by kumaraditya303 · Pull Request #31261 · python/cpython

Conversation

@kumaraditya303

@kumaraditya303 kumaraditya303 commented Feb 10, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 10, 2022
@bedevere-bot bedevere-bot added the awaiting review label Feb 10, 2022
@kumaraditya303
kumaraditya303 force-pushed the global-strings branch from a173ee9 to 3bd4e2a Compare February 10, 2022 14:44
@ericsnowcurrently ericsnowcurrently mentioned this pull request Feb 14, 2022
bpo-46541: Discover the global strings. #31346
Merged
@ericsnowcurrently

ericsnowcurrently commented Feb 14, 2022

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ericsnowcurrently added a commit that referenced this pull request Feb 15, 2022
@ericsnowcurrently
bpo-46541: Discover the global strings. (gh-31346)
12360aa 
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

gvanrossum commented Feb 20, 2022

Copy link
Copy Markdown
Member

@kumaraditya303 Could you fix the conflicts?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303
use global strings
e959ed6
@kumaraditya303
kumaraditya303 force-pushed the global-strings branch from 470037f to e959ed6 Compare February 24, 2022 07:50
@blurb-it
📜🤖 Added by blurb_it.
aa08bfb
@kumaraditya303
kumaraditya303 marked this pull request as ready for review February 24, 2022 07:52
@kumaraditya303

kumaraditya303 commented Feb 24, 2022

Copy link
Copy Markdown
Contributor Author

@kumaraditya303 Could you fix the conflicts?

Fixed
All reactions

Sorry, something went wrong.

Uh oh!

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

kumaraditya303 commented Feb 25, 2022

Copy link
Copy Markdown
Contributor Author

@gvanrossum This is ready for review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gvanrossum
gvanrossum approved these changes Feb 25, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

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.

LGTM.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/scripts/deepfreeze.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Feb 25, 2022
@kumaraditya303
isort
adc8bab
gvanrossum
gvanrossum approved these changes Feb 25, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

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.

Thanks. Waiting for the tests to run once more.

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

Sorry, something went wrong.

Uh oh!

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

kumaraditya303 commented Feb 25, 2022

Copy link
Copy Markdown
Contributor Author

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

Okay, will create a PR tomorrow.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum
gvanrossum merged commit eb002db into python:main Feb 25, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Feb 25, 2022
@gvanrossum

gvanrossum commented Feb 25, 2022

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303
kumaraditya303 deleted the global-strings branch February 25, 2022 18:33
asvetlov pushed a commit that referenced this pull request Feb 26, 2022
@kumaraditya303 @asvetlov
bpo-46712: Share global string identifiers in deepfreeze (GH-31261)
31fa802 
Where appropriate, deepfreeze.c now uses `&_Py_ID(blah)` references instead of locally defining constants. This saves some space.
@ericsnowcurrently ericsnowcurrently mentioned this pull request Mar 1, 2022
bpo-46712: Let generate_global_objects.py Run on Earlier Python Versions #31637
Merged
@ericsnowcurrently ericsnowcurrently mentioned this pull request Jul 9, 2022
Replace _Py_IDENTIFIER() with statically initialized objects. #90699
Closed
@kumaraditya303 kumaraditya303 mentioned this pull request Apr 10, 2022
Share global string identifiers in deepfreeze #90868
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

5 participants

@kumaraditya303 @ericsnowcurrently @gvanrossum @the-knights-who-say-ni @bedevere-bot
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.