NotificationsYou must be signed in to change notification settings
Fork32.1k
Star67.4k

bpo-46443: deepfreeze use preallocated small ints#30715

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

gvanrossum merged 4 commits intopython:mainfromkumaraditya303:longobj

Jan 20, 2022

Merged

bpo-46443: deepfreeze use preallocated small ints#30715

gvanrossum merged 4 commits intopython:mainfromkumaraditya303:longobj

Jan 20, 2022

Conversation

Copy link

Contributor

kumaraditya303 commentedJan 20, 2022•
edited by bedevere-bot
Loading

https://bugs.python.org/issue46443

deepfreeze use preallocated small ints

7fb3f22

kumaraditya303 mentioned this pull request

Jan 20, 2022

Look into string sharing for deep-frozen codefaster-cpython/ideas#218

Closed

Copy link

ContributorAuthor

kumaraditya303 commentedJan 20, 2022

It is an implementation detail so skip news.

Copy link

Member

gvanrossum commentedJan 20, 2022

It is an implementation detail so skip news.

I don't think that's how we usually do this. We skip news for things like fixing typos in comments, but almost anything else gets a new entry. Maybe you're confused with "What's New in 3.11"? That definitely only should get the major visible items.

gvanrossum reviewed

Jan 20, 2022

View reviewed changes

Copy link

Member

gvanrossum left a comment

Choose a reason for hiding this comment

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

LG except I would like to see a one-line news item.

Did you check how much space this saves? (Probably nut that much since ints are small.)

Tools/scripts/deepfreeze.pyShow resolvedHide resolved

Copy link

ContributorAuthor

kumaraditya303 commentedJan 20, 2022

It is an implementation detail so skip news.
I don't think that's how we usually do this. We skip news for things like fixing typos in comments, but almost anything else gets a new entry. Maybe you're confused with "What's New in 3.11"? That definitely only should get the major visible items.

Ok, I'll add a news fragment. Should I create a bpo for this now ?

Copy link

Member

gvanrossum commentedJan 20, 2022

Should I create a bpo for this now ?

Yeah.

kumaraditya303 changed the title~~deepfreeze use preallocated small ints~~bpo-46443: deepfreeze use preallocated small ints

Jan 20, 2022

Copy link

Member

gvanrossum commentedJan 20, 2022

I recommend that you put a little more effort in writing the initial comment for the bpo issues you create. E.g. not just what to do, but why it's worth doing and what impact you expect.

📜🤖 Added by blurb_it.

462b899

gvanrossum approved these changes

Jan 20, 2022

View reviewed changes

Copy link

Member

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! This was indeed super-simple. :-)

bedevere-bot added the awaiting merge label

Jan 20, 2022

Copy link

ContributorAuthor

kumaraditya303 commentedJan 20, 2022•
edited
Loading

I would also do this for empty bytes object as that also has a global singleton. (DONE)

Copy link

Member

gvanrossum commentedJan 20, 2022

I would also do this for empty bytes object as that also has a global singleton but separately.

Good idea. Maybe you can just add that to this PR, it's small enough.

kumaraditya303 added2 commits

January 20, 2022 11:13

use cached empty bytes object

1dfecc0

Merge branch 'longobj' ofhttps://github.com/kumaraditya303/cpythoni…

b55621b

…nto longobj

Copy link

ContributorAuthor

kumaraditya303 commentedJan 20, 2022

It is an implementation detail so skip news.
I don't think that's how we usually do this. We skip news for things like fixing typos in comments, but almost anything else gets a new entry. Maybe you're confused with "What's New in 3.11"? That definitely only should get the major visible items.

I was confused by this PR as it skipped news#30092