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-101430: Update tracemalloc to handle presize properly. by corona10 · Pull Request #101745 · python/cpython

Conversation

@corona10

@corona10 corona10 commented Feb 9, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Issue: _tracemalloc__get_object_traceback doesn't handle objects with pre-headers correctly #101430

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@corona10
corona10 requested a review from colesbury February 9, 2023 16:16
@bedevere-bot bedevere-bot added the awaiting core review label Feb 9, 2023
@bedevere-bot bedevere-bot mentioned this pull request Feb 9, 2023
_tracemalloc__get_object_traceback doesn't handle objects with pre-headers correctly #101430
Closed
@corona10

corona10 commented Feb 9, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

AS-IS

# ./python.exe -Wd -X tracemalloc=2 gh-101430.py
/Users/user/oss/cpython/gh-101430.py:5: ResourceWarning: Uh oh
  warnings.warn("Uh oh", ResourceWarning, source=self)

TO-BE

# ./python.exe -Wd -X tracemalloc=2 gh-101430.py
/Users/user/oss/cpython/gh-101430.py:5: ResourceWarning: Uh oh
  warnings.warn("Uh oh", ResourceWarning, source=self)
Object allocated at (most recent call last):
  File "/Users/user/oss/cpython/gh-101430.py", lineno 10
    func()
  File "/Users/user/oss/cpython/gh-101430.py", lineno 8
    m = MyClass()
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10
pythongh-101430: Update tracemalloc to handle presize properly.
6b451fa
@corona10
corona10 force-pushed the gh-101430 branch from f3808bf to 6b451fa Compare February 9, 2023 16:18
@corona10

corona10 commented Feb 9, 2023

Copy link
Copy Markdown
Member Author

I now read all the spec of the preheader from #29879.
Looks like preheader should be considered when the PyType_IS_GC is true.
All reactions

Sorry, something went wrong.

Uh oh!

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

corona10 commented Feb 9, 2023

Copy link
Copy Markdown
Member Author

@colesbury PTAL
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colesbury
colesbury reviewed Feb 9, 2023
View reviewed changes
Comment thread Modules/_tracemalloc.c Outdated
type = Py_TYPE(obj);
if (PyType_IS_GC(type)) {
ptr = (void *)((char *)obj - sizeof(PyGC_Head));
const size_t presize = _PyType_PreHeaderSize(type);

@colesbury colesbury Feb 9, 2023

Copy link
Copy Markdown
Contributor

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.

This looks correct, but I think it would be more straightforward if you skip the PyType_IS_GC check (and always subtract the pre-header size.) _PyType_PreHeaderSize returns zero for non-GC types.

For context, here's an example in _PyType_AllocNoTrack that operates on GC and non-GC types and unconditionally adds the pre-header size:

cpython/Objects/typeobject.c

Lines 1300 to 1301 in ecfd2d3

const size_t presize = _PyType_PreHeaderSize(type);
char *alloc = PyObject_Malloc(size + presize);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
corona10 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@colesbury

colesbury commented Feb 9, 2023

Copy link
Copy Markdown
Contributor

@corona10 There's also one more tracemalloc related use of sizeof(PyGC_Head) that I missed in the bug report (_PyMem_DumpTraceback calls into tracemalloc):

cpython/Objects/object.c

Lines 2390 to 2398 in ecfd2d3

void *ptr;
PyTypeObject *type = Py_TYPE(obj);
if (_PyType_IS_GC(type)) {
ptr = (void *)((char *)obj - sizeof(PyGC_Head));
}
else {
ptr = (void *)obj;
}
_PyMem_DumpTraceback(fileno(stderr), ptr);

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10
Address code review
a6889be
@corona10

corona10 commented Feb 9, 2023

Copy link
Copy Markdown
Member Author

@corona10 There's also one more tracemalloc related use of sizeof(PyGC_Head) that I missed in the bug report (_PyMem_DumpTraceback calls into tracemalloc):

Thank you for catching all of them!
colesbury and gpshead reacted with heart emoji
All reactions
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10
corona10 requested review from gpshead and pablogsal February 9, 2023 17:06
@gpshead
gpshead requested a review from vstinner February 9, 2023 20:16
gpshead
gpshead approved these changes Feb 9, 2023
View reviewed changes

@gpshead gpshead 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.

I believe this is good, but I've looped in @vstinner for an extra pair of expert eyes on this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Feb 9, 2023
@corona10

corona10 commented Feb 9, 2023

Copy link
Copy Markdown
Member Author

@gpshead

AFAIK, Victor is enjoying his detoxing for OSS.
I will merge the PR since there looks like no serious issue.

cc @vstinner
If there needs something to fix, please let me know :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10
corona10 merged commit 5b946d3 into python:main Feb 9, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Feb 9, 2023
@corona10
corona10 deleted the gh-101430 branch February 9, 2023 23:30
carljm added a commit to carljm/cpython that referenced this pull request Feb 10, 2023
@carljm
Merge branch 'main' into inlinecomp2
add772e 
* main:
  Fix some typos in asdl_c.py (pythonGH-101757)
  pythongh-101747: Fix refleak in new `OrderedDict` repr (pythonGH-101748)
  pythongh-101430: Update tracemalloc to handle presize properly. (pythongh-101745)
  pythonGH-101228: Fix typo in docstring for read method of `_io.TextIOWrapper` class (python#101227)
  Fix typo in `test_fstring.py` (python#101600)
  pythongh-101726: Update the OpenSSL version to 1.1.1t (pythonGH-101727)
  pythongh-101283: Fix 'versionchanged' for the shell=True fallback on Windows in 3.12 (pythonGH-101728)
  LibFFI build requires x64 Cygwin, and skip the ARM build (pythonGH-101743)
@vstinner

vstinner commented Apr 5, 2023

Copy link
Copy Markdown
Member

Thanks for this nice fix @corona10! _PyType_PreHeaderSize() is convenient and better than previously copied-pasted code ;-)
corona10 reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury left review comments
@gpshead gpshead gpshead approved these changes
@pablogsal pablogsal Awaiting requested review from pablogsal
@vstinner vstinner Awaiting requested review from vstinner

Uh oh!

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

Assignees

No one assigned

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

@corona10 @colesbury @vstinner @gpshead @bedevere-bot
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 01402503 — 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.