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 — gh-101266: Fix __sizeof__ for subclasses of int by mdickinson · Pull Request #101394 · python/cpython

Conversation

@mdickinson

@mdickinson mdickinson commented Jan 28, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

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

  • Issue: int and set subclass __sizeof__ under-reports the instance dictionary pointer #101266

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mdickinson
Fix __sizeof__ for subclasses of int
c345b90
@mdickinson mdickinson added needs backport to 3.10 needs backport to 3.11 only security fixes labels Jan 28, 2023
@bedevere-bot bedevere-bot added the awaiting core review label Jan 28, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jan 28, 2023
int and set subclass __sizeof__ under-reports the instance dictionary pointer #101266
Closed
@mdickinson

mdickinson commented Jan 28, 2023

Copy link
Copy Markdown
Member Author

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

@corona10 Do you have enough spare cycles to review?
corona10 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mdickinson
mdickinson requested a review from corona10 January 28, 2023 13:26
mdickinson added 2 commits January 28, 2023 13:27
@mdickinson
Fix markup
d8c8f32
@mdickinson
Preserve existing comment
f46ba55
@mdickinson

mdickinson commented Jan 28, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mdickinson
mdickinson marked this pull request as draft January 28, 2023 14:01
@mdickinson
Fix tp_basicsize and tp_itemsize for bool
b7eba00
@mdickinson

mdickinson commented Jan 28, 2023

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mdickinson
Fix missing include
9cd5d01
@corona10

corona10 commented Jan 28, 2023

Copy link
Copy Markdown
Member

Do you have enough spare cycles to review?

Thanks I will take a look
mdickinson reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mdickinson
mdickinson marked this pull request as ready for review January 28, 2023 14:45
@mdickinson mdickinson removed the needs backport to 3.10 label Jan 28, 2023
@corona10 corona10 self-assigned this Jan 29, 2023
@corona10

corona10 commented Jan 30, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10 corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 30, 2023
@bedevere-bot

bedevere-bot commented Jan 30, 2023

Copy link
Copy Markdown

🤖 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 30, 2023
mdickinson added 2 commits January 30, 2023 17:18
@mdickinson
Merge remote-tracking branch 'upstream/main' into fix-sizeof-for-int-…
4cecaba 
…subtypes
@mdickinson
Update for recent changes in longobject.c
ae6229b
@mdickinson

mdickinson commented Jan 30, 2023

Copy link
Copy Markdown
Member Author

@corona10

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

Thanks! Updated.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10 corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 31, 2023
@bedevere-bot

bedevere-bot commented Jan 31, 2023

Copy link
Copy Markdown

🤖 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 31, 2023
corona10
corona10 approved these changes Jan 31, 2023
View reviewed changes

@corona10 corona10 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,

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot removed the awaiting core review label Jan 31, 2023
@bedevere-bot bedevere-bot added the awaiting merge label Jan 31, 2023
@mdickinson

mdickinson commented Jan 31, 2023

Copy link
Copy Markdown
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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

corona10 commented Jan 31, 2023

Copy link
Copy Markdown
Member

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

Great! Feel free to merge anytime you want :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mdickinson
mdickinson merged commit 39017e0 into python:main Feb 5, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Feb 5, 2023
@miss-islington

miss-islington commented Feb 5, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington

miss-islington commented Feb 5, 2023

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington miss-islington assigned mdickinson and unassigned corona10 Feb 5, 2023
@mdickinson

mdickinson commented Feb 5, 2023

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mdickinson mdickinson mentioned this pull request Feb 5, 2023
[3.11] gh-101266: Fix __sizeof__ for subclasses of int (GH-101394) #101579
Merged
@bedevere-bot

bedevere-bot commented Feb 5, 2023

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Feb 5, 2023
@mdickinson

mdickinson commented Feb 5, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mdickinson added a commit that referenced this pull request Feb 5, 2023
@mdickinson
[3.11] gh-101266: Fix __sizeof__ for subclasses of int (GH-101394) (#…
cf89c16 
…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
Revert "[3.11] pythongh-101266: Fix __sizeof__ for subclasses of int (p…
2c5ae16 
…ythonGH-101394) (python#101579)"

This reverts commit cf89c16.
mdickinson added a commit that referenced this pull request Feb 7, 2023
@mdickinson
[3.11] gh-101266: Revert fix __sizeof__ for subclasses of int (#101638)
358b02d 
Revert "[3.11] gh-101266: Fix __sizeof__ for subclasses of int (GH-101394) (#101579)"

This reverts commit cf89c16.
@sobolevn

sobolevn commented Feb 12, 2023

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sobolevn sobolevn mentioned this pull request Feb 12, 2023
Refleak in test_importlib on aarch64 RHEL8 #101766
Closed
@mdickinson

mdickinson commented Feb 12, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

@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.
All reactions

Sorry, something went wrong.

Uh oh!

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

sobolevn commented Feb 12, 2023

Copy link
Copy Markdown
Member

Yes, seems suspicious. I will investigate!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sobolevn added a commit to sobolevn/cpython that referenced this pull request Feb 12, 2023
@sobolevn
Revert "pythongh-101266: Fix __sizeof__ for subclasses of int (python…
357c542 
…#101394)"

This reverts commit 39017e0.
@sobolevn sobolevn mentioned this pull request Feb 12, 2023
Finding a test_importlib refleak on aarch64 RHEL8 #101394 #101847
Closed
@hroncok hroncok mentioned this pull request Mar 2, 2023
Cython 0.29.x Python 3.12.0a5+ tracker cython/cython#5285
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@corona10 corona10 corona10 approved these changes

Uh oh!

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

Assignees

@mdickinson mdickinson

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

@mdickinson @corona10 @bedevere-bot @miss-islington @sobolevn
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 01402520 — 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.