FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-101430: Update tracemalloc to handle presize properly. by corona10 · Pull Request #101745 · python/cpython · GitHub

/ cpython Public

gh-101430: Update tracemalloc to handle presize properly. - #101745

Merged
corona10 merged 2 commits into
python:mainfrom
corona10:gh-101430
Feb 9, 2023
ConversationCommits (2)ChecksFiles changed
Merged

gh-101430: Update tracemalloc to handle presize properly.#101745
corona10 merged 2 commits into
python:mainfrom
corona10:gh-101430

Conversation

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

Copy link
Copy Markdown
Member

corona10 requested a review from colesbury February 9, 2023 16:16
bedevere-bot mentioned this pull request Feb 9, 2023
Closed

corona10 commented Feb 9, 2023 •
edited
Loading

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()

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.

corona10 commented Feb 9, 2023

Copy link
Copy Markdown
Member Author

@colesbury PTAL

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 Feb 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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);

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);

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!

corona10 requested review from gpshead and pablogsal February 9, 2023 17:06
gpshead requested a review from vstinner February 9, 2023 20:16
gpshead approved these changes Feb 9, 2023
View reviewed changes

gpshead left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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 :)

corona10 merged commit 5b946d3 into python:main Feb 9, 2023
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
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 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 ;-)

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL