Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-30054: Expose tracemalloc C API #1236

Merged
merged 1 commit into from Jun 20, 2017
Merged

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


.. c:function: int PyTraceMalloc_Track(unsigned int domain, uintptr_t ptr, size_t size)

Track an allocated memory block in the tracemalloc 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.

: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

=================

.. versionadded:: 3.7

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.

None yet

6 participants