Skip to content

gh-101266: Fix __sizeof__ for subclasses of int #101394

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

Merged
merged 7 commits into from
Feb 5, 2023

Conversation

mdickinson
Copy link
Member

@mdickinson mdickinson commented Jan 28, 2023

This PR fixes the behaviour of the __sizeof__ method (and hence the results returned by sys.getsizeof) for subclasses of int.

@mdickinson
Copy link
Member Author

@ionite34 This should fix the int portion of the bug you reported.

@corona10 Do you have enough spare cycles to review?

@mdickinson mdickinson requested a review from corona10 January 28, 2023 13:26
@mdickinson
Copy link
Member Author

Darnit; this fails for bool; looks like we'll have to special case in boolobject.c. Working on it.

@mdickinson mdickinson marked this pull request as draft January 28, 2023 14:01
@mdickinson
Copy link
Member Author

this fails for bool

That's because we had somewhat bogus numbers for tp_basicsize and tp_itemsize for the bool class. Now fixed.

I'm going to remove the "backport to 3.10" label: the bug does technically still exist in 3.10, but it's messier to fix - it looks like we would need to add a dedicated __sizeof__ implementation for bool in that case, and it doesn't seem worth the effort.

@corona10
Copy link
Member

Do you have enough spare cycles to review?

Thanks I will take a look

@mdickinson mdickinson marked this pull request as ready for review January 28, 2023 14:45
@mdickinson mdickinson removed the needs backport to 3.10 only security fixes label Jan 28, 2023
@corona10 corona10 self-assigned this Jan 29, 2023
@corona10
Copy link
Member

@mdickinson We have to resolve the conflict before reviewing this PR.
Mark Shannon refactored the longobject ;)
c1b1f51

@corona10 corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 30, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @corona10 for commit 9cd5d01 🤖

If you want to schedule another build, you need to add the :hammer: test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 30, 2023
@mdickinson
Copy link
Member Author

@corona10

@mdickinson We have to resolve the conflict before reviewing this PR.

Thanks! Updated.

@corona10 corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 31, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @corona10 for commit ae6229b 🤖

If you want to schedule another build, you need to add the :hammer: test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 31, 2023
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

lgtm,

Just one more thing,
Would you like to change the PyBool_Type initialization with C99 style through this chance?

PyTypeObject PyBool_Type = {
    PyVarObject_HEAD_INIT(&PyType_Type, 0)
    "bool",
     .tp_basicsize = offsetof(struct _longobject, long_value.ob_digit),
     .tp_itemsize = sizeof(digit),
     .tp_dealloc = bool_dealloc,
      ...
     .tp_vectorcall = bool_vectorcall

@mdickinson
Copy link
Member Author

Would you like to change the PyBool_Type initialization with C99 style through this chance?

Good idea! It may make sense to do that in a separate PR, though: I was hoping to backport this bugfix PR to 3.11, and the C99 style change sounds like a cosmetic update that shouldn't be backported.

@corona10
Copy link
Member

It may make sense to do that in a separate PR,

Great! Feel free to merge anytime you want :)

@mdickinson mdickinson merged commit 39017e0 into python:main Feb 5, 2023
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

Sorry, @mdickinson, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 39017e04b55d4c110787551dc9a0cb753f27d700 3.11

@miss-islington miss-islington assigned mdickinson and unassigned corona10 Feb 5, 2023
@mdickinson
Copy link
Member Author

Hmm. Looks like I'll need to do a manual cherry-pick backport to 3.11, excluding the last few commits. I'll do that later today.

@bedevere-bot
Copy link

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

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Feb 5, 2023
@mdickinson
Copy link
Member Author

See #101579 for a manual backport to 3.11 (cherry-picking the commits up to but excluding the main merge).

mdickinson added a commit that referenced this pull request Feb 5, 2023
…101579)

Fix the behaviour of the `__sizeof__` method (and hence the results returned
by `sys.getsizeof`) for subclasses of `int`. Previously, `int` subclasses gave
identical results to the `int` base class, ignoring the presence of the instance
dictionary.

(Manual backport of #101394 to the Python 3.11 branch.)
mdickinson added a commit to mdickinson/cpython that referenced this pull request Feb 7, 2023
mdickinson added a commit that referenced this pull request Feb 7, 2023
Revert "[3.11] gh-101266: Fix __sizeof__ for subclasses of int (GH-101394) (#101579)"

This reverts commit cf89c16.
@sobolevn
Copy link
Member

This PR was the first one to fail refleak bot (test_importlib for some reason): https://buildbot.python.org/all/#/builders/802/builds/582

See #101766

@mdickinson
Copy link
Member Author

mdickinson commented Feb 12, 2023

@sobolevn

Hmm. We could try reverting it in a PR, and then running the refleak buildbots on that PR to see what happens. It seems rather implausible that it was this particular PR that introduced the refleak, though.

@sobolevn
Copy link
Member

Yes, seems suspicious. I will investigate!

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