Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

bpo-30054: Expose tracemalloc C API#1236

Merged
vstinner merged 1 commit intopython:masterpython/cpython:masterfrom
vstinner:tracemallocCopy head branch name to clipboard
Jun 20, 2017
Merged

bpo-30054: Expose tracemalloc C API#1236
vstinner merged 1 commit intopython:masterpython/cpython:masterfrom
vstinner:tracemallocCopy head branch name to clipboard

Conversation

@vstinner
Copy link
Member

  • Make PyTraceMalloc_Track() and PyTraceMalloc_Untrack() functions
    public (remove the "_" prefix)
  • Remove the _PyTraceMalloc_domain_t type: use directly unsigned
    int.
  • Document methods

Note: methods are already tested in test_tracemalloc.

@mention-bot
Copy link

@Haypo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tim-one, @loewis and @mdickinson to be potential reviewers.

@vstinner
Copy link
Member Author

cc @jtaylor

Copy link
Member Author

Choose a reason for hiding this comment

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

:mod:tracemalloc can be used to create a link to the module.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum, the tracemalloc doc and this doc should mention that the tracemalloc uses the default 0 to trace Python memory allocations.

Copy link
Member Author

@vstinner vstinner Jun 14, 2017

Choose a reason for hiding this comment

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

I documented the domain 0 in the tracemalloc doc.

@njsmith
Copy link
Contributor

njsmith commented Jun 10, 2017

This looks good to me, but can you add backcompat for the underscore-prefixed versions of _PyTraceMalloc_Track and _PyTraceMalloc_Untrack? Like #define _PyTraceMalloc_Track PyTraceMalloc_Track? See bpo-30623) for discussion. (Numpy doesn't reference _PyTraceMalloc_domain_t so dropping it is fine with us.)

@juliantaylor
Copy link

numpy doesn't need the backward compatibility symbols, we just need this merged.

@njsmith
Copy link
Contributor

njsmith commented Jun 11, 2017

It's true that if you can commit to merging this in the next few days, before numpy makes its next point release, then the backcompat part will become unnecessary. But I don't want to assume that...

@charris
Copy link

charris commented Jun 11, 2017

@njsmith I like to wait at least two weeks after the first, aka FU, release. It takes that long for the initial bugs to get reported.

@vstinner
Copy link
Member Author

Sorry, I don't understand what I should do.

We don't provide any warranty on the API stability, _Py functions can be removed between releases, even minor releases.

But I can also understand that you don't want to break your CI.

@vstinner
Copy link
Member Author

I rebased my PR and addresed my own 2 comments ;-) I documented the default domain (domain 0).

@njsmith
Copy link
Contributor

njsmith commented Jun 14, 2017

@Haypo: it's not even our CI, it's people who are running their CI on python nightly builds (as recommended here), and are now filing bugs on CPython and numpy. Or are silently giving up on testing cpython nightlies, which is unfortunate because in general I think we like it when people test cpython nightly builds :-).

Obviously there can't be any guarantees between 3.x and 3.(x+1), but it is nice to avoid or minimize these problems when we can.

In this case: if you are sure you want to merge this for 3.7, then it would be very helpful if you can merge it now-ish; then we'll know not to add any compatibility hacks in the next numpy point release. OTOH if you aren't sure about merging it and want to put it off for later, then it would be helpful to let us know, so we can figure out how to set things up so that it doesn't break things again when you do merge it.

wwrechard added a commit to wwrechard/pydlm that referenced this pull request Jun 15, 2017
@vstinner
Copy link
Member Author

Obviously there can't be any guarantees between 3.x and 3.(x+1), but it is nice to avoid or minimize these problems when we can.

I added a compatibility layer to pymem.h:

/* Two macros kept for backport compatibility with Python 3.6. These macros are
   likely to go away before Python 3.7 final release. Their main reason to
   exist is to not break the numpy CI. */
#define _PyTraceMalloc_Track PyTraceMalloc_Track
#define _PyTraceMalloc_Untrack PyTraceMalloc_Untrack
#define _PyTraceMalloc_domain_t unsigned int

@juliantaylor
Copy link

note numpy does not need those compatibility macros, it can already deal with the name change, assuming this gets merged.

* Make PyTraceMalloc_Track() and PyTraceMalloc_Untrack() functions
  public (remove the "_" prefix)
* Remove the _PyTraceMalloc_domain_t type: use directly unsigned
  int.
* Document methods

Note: methods are already tested in test_tracemalloc.
@vstinner
Copy link
Member Author

note numpy does not need those compatibility macros, it can already deal with the name change, assuming this gets merged.

Hum, ok. I removed the compatibility layer since I dislike it, and again, we don't warranty anything on private _PyXXX() functions.

@vstinner vstinner merged commit 5ea4c06 into python:master Jun 20, 2017
@vstinner vstinner deleted the tracemalloc branch June 20, 2017 15:46
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.

6 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.