gh-100637: Fix int and bool __sizeof__ calculation to include the 1 element ob_digit array for 0 and False by ionite34 · Pull Request #100663 · python/cpython

ionite34 · 2023-01-01T15:25:30Z

Fixes behavior where int and bool __sizeof__ under-reports true size by 4 bytes as it did not take into account the size 1 ob_digit array for 0 and False.

Changes

88e249b Use Py_MAX to use the maximum of 1 or the absolute ob_size size.

offsetof(PyLongObject, ob_digit) + Py_MAX(Py_ABS(Py_SIZE(self)), 1)*sizeof(digit)

Testing

391ec87 Update sys.getsizeof unit test for int 0 to use the same size as the cases for -1 and 1
88e249b Add a sys.getsizeof unit test for False matching the previous test case for True

Issue: __sizeof__ incorrectly ignores the size 1 array in PyVarObjects (bool, int, tuple) when ob_size is 0 #100637

Fixes behavior where int (and subtypes like bool) __sizeof__ under-reports true size as it did not take into account the size 1 `ob_digit` array for empty ints.

bedevere-bot · 2023-01-01T15:25:36Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

ghost · 2023-01-01T15:25:57Z

All commit authors signed the Contributor License Agreement.

mdickinson

This looks good to me, and works as advertised in manual testing - thank you!

I've made one nitpick level suggestion for a stylistic change.

(Actually, thinking about style, I'm not sure why the body of this function doesn't simply consist of a single return - the res temporary seems unnecessary.)

Objects/longobject.c

Co-authored-by: Mark Dickinson <dickinsm@gmail.com>

mdickinson

LGTM!

I'll leave this open for a bit in case anyone else has comments (especially because it's New Year's Day and some potential commenters may be AFK), but I think this is ready to merge.

ionite34 · 2023-01-01T17:04:26Z

(Actually, thinking about style, I'm not sure why the body of this function doesn't simply consist of a single return - the res temporary seems unnecessary.)

Thanks for the review and line split suggestion! I'm not really sure about the res over plain return either. Maybe there was a diff minimizing rationale for that original res that I'm not aware of. Currently seems fine though.

ionite34 · 2023-01-03T00:07:33Z

Would this be something that needs backport to 3.10/3.11? Not sure what the policy is for that.

miss-islington · 2023-01-03T15:35:03Z

Thanks @ionite34 for the PR, and @mdickinson for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

…he 1 element ob_digit array for 0 and False (pythonGH-100663) Fixes behaviour where int (and subtypes like bool) __sizeof__ under-reports true size as it did not take into account the size 1 `ob_digit` array for the zero int. (cherry picked from commit d7e7f79) Co-authored-by: Ionite <dev@ionite.io> Co-authored-by: Mark Dickinson <dickinsm@gmail.com>

bedevere-bot · 2023-01-03T15:35:16Z

GH-100717 is a backport of this pull request to the 3.11 branch.

mdickinson · 2023-01-03T15:36:22Z

Would this be something that needs backport to 3.10/3.11? Not sure what the policy is for that.

It's a judgement call, I think: it's arguably a bugfix, but an almost entirely inconsequential one - I can't imagine much real world code that would benefit, even though it's nice to have the fix going forward.

I'll backport to 3.11: Python 3.11 was the version that deliberately started always allocating at least one digit (the relevant commit was 15d50d7). That isn't true in Python 3.10.

(3.10 code:

cpython/Objects/longobject.c

Lines 138 to 139 in a639493

    
           result = PyObject_Malloc(offsetof(PyLongObject, ob_digit) + 
        
                                    size*sizeof(digit));

)
(3.11 code:

cpython/Objects/longobject.c

Lines 155 to 162 in b99ac1d

    
               Py_ssize_t ndigits = size ? size : 1; 
        
               /* Number of bytes needed is: offsetof(PyLongObject, ob_digit) + 
        
                  sizeof(digit)*size.  Previous incarnations of this code used 
        
                  sizeof(PyVarObject) instead of the offsetof, but this risks being 
        
                  incorrect in the presence of padding between the PyVarObject header 
        
                  and the digits. */ 
        
               result = PyObject_Malloc(offsetof(PyLongObject, ob_digit) + 
        
                                        ndigits*sizeof(digit));

)

…the 1 element ob_digit array for 0 and False (GH-100663) (#100717) gh-100637: Fix int and bool __sizeof__ calculation to include the 1 element ob_digit array for 0 and False (GH-100663) Fixes behaviour where int (and subtypes like bool) __sizeof__ under-reports true size as it did not take into account the size 1 `ob_digit` array for the zero int. (cherry picked from commit d7e7f79) Co-authored-by: Ionite <dev@ionite.io> Co-authored-by: Mark Dickinson <dickinsm@gmail.com>

ionite34 added 3 commits January 1, 2023 23:08

Fix int __sizeof__ to use a minimum absolute ob_size of 1

88e249b

Fixes behavior where int (and subtypes like bool) __sizeof__ under-reports true size as it did not take into account the size 1 `ob_digit` array for empty ints.

Update sizeof test for fix on int 0

391ec87

Add sizeof test for False to have same size as True

0726feb

bedevere-bot added the awaiting review label Jan 1, 2023

bedevere-bot mentioned this pull request Jan 1, 2023

__sizeof__ incorrectly ignores the size 1 array in PyVarObjects (bool, int, tuple) when ob_size is 0 #100637

Closed

kumaraditya303 requested a review from mdickinson January 1, 2023 15:31

📜🤖 Added by blurb_it.

7792582

mdickinson reviewed Jan 1, 2023

View reviewed changes

Objects/longobject.c Outdated Show resolved Hide resolved

int __sizeof__ format for line length. Add comment on Py_MAX reasoning.

ab1a78e

Co-authored-by: Mark Dickinson <dickinsm@gmail.com>

mdickinson approved these changes Jan 1, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jan 1, 2023

mdickinson merged commit d7e7f79 into python:main Jan 2, 2023

bedevere-bot removed the awaiting merge label Jan 2, 2023

ionite34 deleted the fix-int-sizeof branch January 3, 2023 02:10

mdickinson added the needs backport to 3.11 only security fixes label Jan 3, 2023

bedevere-bot removed the needs backport to 3.11 only security fixes label Jan 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

gh-100637: Fix int and bool sizeof calculation to include the 1 element ob_digit array for 0 and False#100663

gh-100637: Fix int and bool sizeof calculation to include the 1 element ob_digit array for 0 and False#100663
mdickinson merged 5 commits intopython:mainfrom
ionite34:fix-int-sizeof

ionite34 commented Jan 1, 2023 •

edited by bedevere-bot

Loading

Uh oh!

bedevere-bot commented Jan 1, 2023

Uh oh!

ghost commented Jan 1, 2023 •

edited by ghost

Loading

Uh oh!

mdickinson left a comment

Uh oh!

Uh oh!

mdickinson left a comment

Uh oh!

ionite34 commented Jan 1, 2023

Uh oh!

ionite34 commented Jan 3, 2023

Uh oh!

miss-islington commented Jan 3, 2023

Uh oh!

bedevere-bot commented Jan 3, 2023

Uh oh!

mdickinson commented Jan 3, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

ionite34 commented Jan 1, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Testing

Uh oh!

bedevere-bot commented Jan 1, 2023

Uh oh!

ghost commented Jan 1, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdickinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdickinson left a comment

Choose a reason for hiding this comment

Uh oh!

ionite34 commented Jan 1, 2023

Uh oh!

ionite34 commented Jan 3, 2023

Uh oh!

miss-islington commented Jan 3, 2023

Uh oh!

bedevere-bot commented Jan 3, 2023

Uh oh!

mdickinson commented Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ionite34 commented Jan 1, 2023 •

edited by bedevere-bot

Loading

ghost commented Jan 1, 2023 •

edited by ghost

Loading

mdickinson commented Jan 3, 2023 •

edited

Loading