Skip to content

GH-148189: Fix miscalculation of type-specific free list memory use#148190

Open
MazinSharaf wants to merge 5 commits intopython:mainfrom
MazinSharaf:bytesizefix
Open

GH-148189: Fix miscalculation of type-specific free list memory use#148190
MazinSharaf wants to merge 5 commits intopython:mainfrom
MazinSharaf:bytesizefix

Conversation

@MazinSharaf
Copy link
Copy Markdown
Contributor

@MazinSharaf MazinSharaf commented Apr 7, 2026

Fixing calculation of bytes in type-specific free lists

GH-148189: Fix miscalculation of type-specific free list memory use

@tim-one tim-one changed the title Fix PyListObject size calculation in allocator GH-148189: Fix miscalculation of type-specific free list memory use Apr 7, 2026
Adjusted memory calculation for PyTupleObject freelist entries.
@tim-one
Copy link
Copy Markdown
Member

tim-one commented Apr 7, 2026

As mentioned in the issue report, don't add the size of the GC header yourself. The layout of objects is subject to change over the time. Use _PyObject_SIZE() instead for lists and dicts. The related _PyObject_VAR_SIZE() is appropriate for tuples, and you're already using that - good!

This also needs a brief NEWS entry.

@tim-one
Copy link
Copy Markdown
Member

tim-one commented Apr 7, 2026

On third thought 😉, I'm not actually sure exactly how to spell this. Bottom line is that the output should match the results given by sys.getsizeof().

@tim-one
Copy link
Copy Markdown
Member

tim-one commented Apr 7, 2026

OK, I was wrong! tp_basicsize does not include GC overhead, sys.getsizef() adds that itself if the type participates in gc. So your original approach appears to be the best approach after all! My apologies

@tim-one
Copy link
Copy Markdown
Member

tim-one commented Apr 7, 2026

OK, I think your new approach is good! Except, perhaps, for tuples. I believe I was wrong that _PyObject_VAR_SIZE() already includes gc overheads. Please check output results against what sys.getsizeof() returns when passed tuples with the same number of elements.

@tim-one
Copy link
Copy Markdown
Member

tim-one commented Apr 7, 2026

Well, I tried to add a NEWS entry, but blurb-it gave me an error:

Cannot create news file for GH-148190! Make sure you have access to that PR!

No idea why 😦

@tim-one
Copy link
Copy Markdown
Member

tim-one commented Apr 8, 2026

Nobody seems to know why I can't add a NEWS entry on your behalf. You'll have to try it. Look for

bedevere/news — No news entry in Misc/NEWS.d/next/ or "skip news" label found

Click on "..." to its right. then "View details" in the pop-up menu. That will take you to the web version of Blurb-it. Fill in the issue and PR numbers (148189 and 148190). select "Core" from the dropdown list, and add text, like

Repaired undercount of bytes in type-specific free lists reported by sys._debugmallocstats(). For types that participate in cyclic garbage collection, it was missing two pointers used by gc.

Then submit it. If all goes well, Blurb-it will magically add another file to this PR.

@MazinSharaf
Copy link
Copy Markdown
Contributor Author

MazinSharaf commented Apr 8, 2026

Ok just to clarify I'll revert the tuple change and add a NEWS entry for it?

Sorry for taking time to respond as well.

MazinSharaf and others added 2 commits April 8, 2026 12:44
Removed unnecessary comment regarding memory calculation and the memory calculation itself.
@webknjaz
Copy link
Copy Markdown
Member

webknjaz commented Apr 8, 2026

@tim-one https://github.com/python/blurb_it?tab=readme-ov-file#about seems to imply that the GH App is expected to be installed in the contributor account, giving access to the fork repo.

// looks like @MazinSharaf figured it out already


You can probably go to the branch in the fork on GH UI and create the file manually, w/o blurb-it, FYI.

@tim-one
Copy link
Copy Markdown
Member

tim-one commented Apr 8, 2026

Yay! The NEWS entry looks good! Although they probably want some reST markup around sys._debugmallocstats() - anyone? That's something I don't know about.

Please check to see that all free-list types in the output deliver the same story as sys.getsizeof(). I only mentioned list, tuple, and dict, but there may be more. tuple definitely needs changes too, but more along the lines of what you already did for list and dict. I steered you wrong before about how to do it. You figured out the right way, so now go wild with it ;-)

A difference is that lists and dicts in free lists are empty, but tuples are in multiple free lists, one for each number of contained elements. The VAR size stuff you did before was fine so far as it went, but needs to add in the "preheader" size too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants