_tracemalloc: add support for multiple address spaces (domains)

[vstinner]

BPO	26588
Nosy	@pitrou, @vstinner, @njsmith
Files	tracemalloc.patch tracemalloc-2.patch tracemalloc-3.patch tracemalloc-4.patch tracemalloc-5.patch hashtable_key_size.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2016-03-25.12:15:40.332>
created_at = <Date 2016-03-18.21:39:26.994>
labels = ['extension-modules', 'type-feature']
title = '_tracemalloc: add support for multiple address spaces (domains)'
updated_at = <Date 2016-03-25.12:15:40.330>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2016-03-25.12:15:40.330>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2016-03-25.12:15:40.332>
closer = 'vstinner'
components = ['Extension Modules']
creation = <Date 2016-03-18.21:39:26.994>
creator = 'vstinner'
dependencies = []
files = ['42207', '42210', '42211', '42232', '42234', '42237']
hgrepos = []
issue_num = 26588
keywords = ['patch']
message_count = 43.0
messages = ['261997', '261998', '262003', '262004', '262005', '262006', '262125', '262127', '262129', '262131', '262132', '262134', '262145', '262148', '262149', '262151', '262152', '262154', '262172', '262177', '262178', '262181', '262185', '262188', '262190', '262198', '262201', '262202', '262205', '262206', '262207', '262210', '262211', '262212', '262214', '262215', '262220', '262221', '262248', '262249', '262250', '262309', '262433']
nosy_count = 4.0
nosy_names = ['pitrou', 'vstinner', 'njs', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue26588'
versions = ['Python 3.6']

[vstinner]

The tracemalloc module uses a hashtable: pointer (void*) => trace. Some embedded devices use multiple address spaces. Each GPU also has its own address space. CUDA, OpenCL and OpenGL should also be seen as different address spaces.

In the issue bpo-26530, it was proposed (required?) to support multiple addresses spaces to be able to use tracemalloc in numpy.

Attached patch enhances tracemalloc to use (pointer: void*, domain: unsigned int) as key in the hashtable. A (pointer, domain) key is stored in a hashtable entry.

In the patch, the domain is hardcoded to 0, but the issue bpo-26530 will add new C functions to track/untrack memory blocks, and this new functions will have a domain parameter.

The patch changes how a key is passed to the hashtable API: pass a *pointer* to a key rather than directly the key value. Because of that, the patch is quite long.

The patch also removes the unused function _Py_hashtable_hash_int().

_Py_HASHTABLE_ENTRY_DATA() macro now requires the hashtable to get the key size, since the offset of data now depends on the key size.

[pitrou]

Le 18/03/2016 22:39, STINNER Victor a écrit :

In the issue bpo-26530, it was proposed (required?) to support multiple
addresses spaces to be able to use tracemalloc in numpy.

Only proposed :-) Numpy itself only works on the CPU, however Nathaniel
may have other use cases of his own. Also, Numba allows use of the Numpy
API (part thereof, anyway) on GPUs.

[vstinner]

Patch version 2:

• modify _tracemalloc._get_traces() to include domain in traces
• add tracemalloc.DomainFilter(domain)
• add domain optional parameter to tracemalloc.Filter constructor

Currently, Snapshot._group_by() ignores domain information. I don't know how the domain should be exposed, *if* it should be exposed.

By the way, maybe we don't need to export the domain at the Python level at all? (Don't expose it in _tracemalloc._get_traces()).

[vstinner]

Also, Numba allows use of the Numpy API (part thereof, anyway) on GPUs.

Do you mean that Numba requires supporting multiple address spaces to be able to use tracemalloc? Or that supporting multiple address spaces would also to *also* track GPU memory allocations?

A friend told me that CUDA, OpenGL and OpenCL can (should?) be seen as 3 different address spaces. These libraries don't expose directly addresses in the GPU address space, but may use handles.

[vstinner]

Patch 3:

• avoid the memory overhead of storing domain if domains are not used
• add optional use_domain parameter to tracemalloc.start()
• by default, no change: use a C void* (pointer) as key for the trace hashtable
• if start() is called with use_domain=True, use (pointer: void*, domain: unsigned int) key for the trace hashtable

[vstinner]

Hum, on OpenStack it's common for me to work on a patch serie. Since our development workflow doesn't support that, I used a single patch. It's hard to review it, all changes are merged into one big patch :-/

[vstinner]

Crap. I pushed tracemalloc-3.patch by mistake. I just reverted my mistake.

At least, it helped to find first bugs using buildbots:

http://buildbot.python.org/all/builders/PPC64%20Fedora%203.x/builds/676/steps/test/logs/stdio

[230/400/3] test_tracemalloc
python: ./Modules/_tracemalloc.c:195: set_reentrant: Assertion `PyThread_get_key_value(tracemalloc_reentrant_key) == ((PyObject *) &_Py_TrueStruct)' failed.
Fatal Python error: Aborted

Current thread 0x00003fffb0a95a10 (most recent call first):
File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/test/test_tracemalloc.py", line 85 in setUp
File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/unittest/case.py", line 596 in run

[vstinner]

Updated patch taking Antoine's comments in account.

Instead of filling pointer_t padding with zeros, I changed the size of the key (key_size) in the hashtable to exclude padding.

[pitrou]

Instead of filling pointer_t padding with zeros, I changed the size of the key (key_size) in the hashtable to exclude padding.

What about alignment issues on strict platforms? (SPARC?)

[vstinner]

What about alignment issues on strict platforms? (SPARC?)

Which kind of alignment issue do you expect?

I added "__attribute__((packed))" to frame_t. I expected SIGBUS because of that (it's even documented in a comment ;-)), but I practice nobdy complained, not even our buildbots. Hum, it looks like we only have one SPARC buildbot and it's offline...

_Py_hashtable_get() uses memcpy() to copy the value, so this function should not have any alignment issue.

The new code uses pointer dereference, so alignment issues are more likely. How can I test it on Intel CPU?

Should I also use memcpy() to retrieve the key value?

[pitrou]

What about compilers that don't support "__attribute__((packed))"? Instead you could use a compare func that compares struct fields...

[vstinner]

Patch version 5:

• add hashtable_compare_pointer_t() which compares pointer_t fields (ptr and domain) rather using _Py_hashtable_compare_direct() which causes issues with padding
• hashtable_hash_pointer_t() now uses memcpy() to get the pointer_t value rather than a pointer dereference

Antoine:

What about compilers that don't support "__attribute__((packed))"?

packed is a small and *optional* optimization on the memory footprint of _tracemalloc structure (reduce tracemalloc.get_tracemalloc_memory()).

Using packed can introduce memory alignment issue, but since tracemalloc uses memcpy(), it *should* be fine in practice. Again, since I don't have access to SPARC CPU, I don't know how to test.

Instead you could use a compare func that compares struct fields...

Done.

[vstinner]

Hum, the patch is really too hard to review. So I extract to the "key_size" part: hashtable_key_size.patch.

I added new macros to get the key, wrapper to memcpy() with an assert to check the key size in debug mode.