{"action":"created","issue":{"url":"https://api.github.com/repos/numpy/numpy/issues/8303","repository_url":"https://api.github.com/repos/numpy/numpy","labels_url":"https://api.github.com/repos/numpy/numpy/issues/8303/labels{/name}","comments_url":"https://api.github.com/repos/numpy/numpy/issues/8303/comments","events_url":"https://api.github.com/repos/numpy/numpy/issues/8303/events","html_url":"https://github.com/numpy/numpy/pull/8303","id":191323210,"number":8303,"title":"ENH: add cyclic GC support to object arrays. Closes #6581.","user":{"login":"J-Sand","id":23218231,"avatar_url":"https://avatars.githubusercontent.com/u/23218231?v=3","gravatar_id":"","url":"https://api.github.com/users/J-Sand","html_url":"https://github.com/J-Sand","followers_url":"https://api.github.com/users/J-Sand/followers","following_url":"https://api.github.com/users/J-Sand/following{/other_user}","gists_url":"https://api.github.com/users/J-Sand/gists{/gist_id}","starred_url":"https://api.github.com/users/J-Sand/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/J-Sand/subscriptions","organizations_url":"https://api.github.com/users/J-Sand/orgs","repos_url":"https://api.github.com/users/J-Sand/repos","events_url":"https://api.github.com/users/J-Sand/events{/privacy}","received_events_url":"https://api.github.com/users/J-Sand/received_events","type":"User","site_admin":false},"labels":[{"id":6359239,"url":"https://api.github.com/repos/numpy/numpy/labels/09%20-%20Enhancement","name":"09 - Enhancement","color":"fef2c0","default":false},{"id":16917029,"url":"https://api.github.com/repos/numpy/numpy/labels/component:%20numpy.core","name":"component: numpy.core","color":"009800","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":6,"created_at":"2016-11-23T16:38:53Z","updated_at":"2016-11-29T20:34:51Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/numpy/numpy/pulls/8303","html_url":"https://github.com/numpy/numpy/pull/8303","diff_url":"https://github.com/numpy/numpy/pull/8303.diff","patch_url":"https://github.com/numpy/numpy/pull/8303.patch"},"body":"There are a few things I'm uncertain about here:\r\n\r\n- Currently `ndarray` has custom `tp_alloc` and `tp_free` functions, but they don't seem to do anything different to the defaults. Combining cyclic garbage support with custom `tp_alloc` and `tp_free` functions is apparently difficult and I couldn't get it to work, so I just got rid of them. Is that acceptable? It doesn't cause any harm as far as I can tell.\r\n\r\n- I don't really understand `UPDATEIFCOPY`. With the more normal use of `base`, the appropriate thing to do is to visit it in `array_traverse` (because `base` can participate in reference cycles) but ignore it in `array_clear` (because you can't have a reference cycle purely consisting of `base`s, AFAIK). I have a feeling we might need to do something special for arrays with `UPDATEIFCOPY` set.\r\n\r\n- the `array_traverse` code is largely copied from `PyArray_INCREF`, and I included the code that deals with discontiguous and misaligned arrays. However, `PyArray_INCREF` and friends need to support arrays that don't own their own data, but `array_traverse` (or at least this section of it) doesn't. Is it possible to have a discontiguous or misaligned object array (or array with object fields) that owns its data? If not, `array_traverse` can be simplified quite a bit.\r\n\r\n- there are couple of places in `array_dealloc` where the array's refcount is incremented to prevent numpy API calls from triggering `array_dealloc` again (which would be catastrophic). This is harmless, but it makes it difficult to debug GC stuff, because it makes the total refcount go up every time an object array (or an array with `UPDATEIFCOPY` set) is created and then collected. Is there a straightforward way of avoiding this? I can't think of anything."},"comment":{"url":"https://api.github.com/repos/numpy/numpy/issues/comments/263690643","html_url":"https://github.com/numpy/numpy/pull/8303#issuecomment-263690643","issue_url":"https://api.github.com/repos/numpy/numpy/issues/8303","id":263690643,"user":{"login":"J-Sand","id":23218231,"avatar_url":"https://avatars.githubusercontent.com/u/23218231?v=3","gravatar_id":"","url":"https://api.github.com/users/J-Sand","html_url":"https://github.com/J-Sand","followers_url":"https://api.github.com/users/J-Sand/followers","following_url":"https://api.github.com/users/J-Sand/following{/other_user}","gists_url":"https://api.github.com/users/J-Sand/gists{/gist_id}","starred_url":"https://api.github.com/users/J-Sand/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/J-Sand/subscriptions","organizations_url":"https://api.github.com/users/J-Sand/orgs","repos_url":"https://api.github.com/users/J-Sand/repos","events_url":"https://api.github.com/users/J-Sand/events{/privacy}","received_events_url":"https://api.github.com/users/J-Sand/received_events","type":"User","site_admin":false},"created_at":"2016-11-29T20:34:51Z","updated_at":"2016-11-29T20:34:51Z","body":"Thanks for the help!\r\n\r\n> It's hard to evaluate \"some getting much faster and some much slower\" though :-)\r\n\r\nI ran the benchmarks with something like `run_tests.py --bench-compare HEAD~2 bench_core`, but now I realise it gives me wildly different results every time. Maybe I need some command-line options to increase the number of loops, or it's intended to be run in environments more stable than my laptop?\r\n\r\nI ran some benchmarks with `perf` on `np.empty` with dtypes of `float` and `object` and shapes of `1` and `100`. The impact seems pretty negligible:\r\n\r\n    before:\r\n    small_empty_float_array: Median +- std dev: 7.21 us +- 0.30 us\r\n    large_empty_float_array: Median +- std dev: 7.13 us +- 0.32 us\r\n    small_empty_object_array: Median +- std dev: 11.8 us +- 0.5 us\r\n    large_empty_object_array: Median +- std dev: 12.8 us +- 0.7 us\r\n\r\n    after:\r\n    small_empty_float_array: Median +- std dev: 7.24 us +- 0.26 us\r\n    large_empty_float_array: Median +- std dev: 7.18 us +- 0.30 us\r\n    small_empty_object_array: Median +- std dev: 11.9 us +- 0.5 us\r\n    large_empty_object_array: Median +- std dev: 13.0 us +- 0.6 us\r\n\r\nThere really shouldn't be much CPU overhead, because tracking/untracking objects just consists of adding/removing them to a doubly linked list. There is no ongoing overhead for untracked objects, and even for object arrays I don't think the traversal should be hugely expensive.\r\n\r\nI think the memory is a bigger problem - allocating 24 extra bytes (at least on 64-bit linux) for every array object, which almost always goes unused, is clearly not ideal.\r\n\r\n> I guess we could smuggle this information in using a thread-local... it's ugly, but depending on how much overhead the GC stuff adds it might be worth it.\r\n\r\nHmm... the biggest problem is that `tp_free` needs to know which free function to call. We can just have `tp_free` do a `PyDataType_REFCHK`, but then we need to be able to guarantee that all arrays were allocated using the appropriate allocator, and third-party C extension code might fail to do that (for example, somebody might have a subtype with a custom `tp_new`, or they might build an `ndarray` by calling its `tp_alloc` directly, bypassing our `tp_new`).\r\n\r\nI think a new flag would work. We could have `tp_alloc` use the non-GC allocator, and everywhere we call `tp_alloc` we could check whether we want an object array, and if so use the GC allocator instead of `tp_alloc` and set the flag. Then `tp_free` could just look at the flag. If an extension module fails to do this dance in the places where they call `tp_alloc`, they won't get an invalid free, they just won't get cyclic GC support (or we could make the GC allocator the default, and they would waste a bit of memory). And if they already bypass our `tp_alloc` altogether, they really should be providing their own `tp_free`, right?\r\n\r\nThat's the best option I can think of anyway."}}
{"actor":{"display_login":"J-Sand"}}