BUG: deallocate recursive closure in arrayprint.py #10621
Conversation
6d3320e to
787c75a
Compare
|
Hmm, my original code here to do I have to admit, I don't understand PyPy vs CPython garrbage collection well enough to see how to cause the closure to get collected immediately. So I'm just going to go for the code reorg here to avoid the closure. All I did is move the closure out to its own function, with a new arg to store the closure variables. |
numpy/core/arrayprint.py
Outdated
| """formatArray is designed for two modes of operation: | ||
| "param" are the values that are invariant under recursion, including | ||
| the array to be printed itself. index, indent and curr_width | ||
| are updated during recursion. |
There was a problem hiding this comment.
A full up documentation string would be helpful here. I didn't worry about that for the backport, but it would be good to have it going forward.
|
Perhaps we should wrap this in a thin class, store the persistent settings
in the class, and have the recursive method be a private member?
…On Sun, Feb 18, 2018, 11:23 Charles Harris ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In numpy/core/arrayprint.py
<#10621 (comment)>:
>
-def _formatArray(a, format_function, line_width, next_line_prefix,
- separator, edge_items, summary_insert, legacy):
- """formatArray is designed for two modes of operation:
+ "param" are the values that are invariant under recursion, including
+ the array to be printed itself. index, indent and curr_width
+ are updated during recursion.
A full up documentation string would be helpful here. I didn't worry about
that for the backport, but it would be good to have it going forward.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#10621 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAZ9LH7BqHtbO6qNFA4NEOkPJ8yxCNsmks5tWHjDgaJpZM4SJbxG>
.
|
787c75a to
e667c0e
Compare
| finally: | ||
| # recursive closures have a cyclic reference to themselves, which | ||
| # requires gc to collect (gh-10620). To avoid this problem, for | ||
| # performance and PyPy friendliness, we break the cycle: |
There was a problem hiding this comment.
Also to avoid UPDATE_IF_COPY interactions, but no need to mention that here
numpy/core/shape_base.py
Outdated
| # recursive closures have a cyclic reference to themselves, which | ||
| # requires gc to collect (gh-10620). To avoid this problem, for | ||
| # performance and PyPy friendliness, we break the cycle: | ||
| recurser = None |
There was a problem hiding this comment.
Should be block_recursion = None?
e667c0e to
50fde71
Compare
|
Fixed up. |
|
Out of curiousity - does using a Y-combinator solve the reference cycle? |
|
Ah, that's a bit over my head, though I would be pleased to understand that on the tips of my fingers. I thought monads were the way of storing hidden state (closure variables). |
|
I guess the key thing here is to get the self-reference via an argument rather than a closure. Without invoking a full y-combinator: def recurser(recurser, outer_args):
...
# do the recursive call
recurser(recurser, inner_args)
recurser(recurser, initial_args)Or with one: def fix(f):
return lambda *args, **kwargs: f(fix(f), *args, **kwargs)
@fix
def recurser(recurser, outer_args):
...
# do the recursive call
recurser(inner_args)
recurser(initial_args) |
Fixes #10620