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

Update postproc to general multiple del nodes if a variable is assigned multiple times in the same basic block #7233

Closed
wants to merge 2 commits into from

Conversation

njriasan
Copy link
Contributor

@njriasan njriasan commented Jul 22, 2021

In Bodo we assign to a variable multiple times to support "inplace" updates. For example a sort with inplace=True is accomplished by reassigning to the same IR variable after a sort is finished. This change fixes a memory leak that was deteced in Numba 0.54 because only 1 del is generated.

I'll add a reproducer with a Numba pipeline extension shortly.

@gmarkall
Copy link
Member

Is this to fix #7225?

@njriasan njriasan marked this pull request as ready for review July 28, 2021 16:37
@njriasan
Copy link
Contributor Author

No this fixes #7258, which I previously described on discourse but hadn't filed the issue. This seems like an easy fix and doesn't seem to interfere with any tests, so could we include it in the numba 0.54 release?

@gmarkall
Copy link
Member

I'll add a reproducer with a Numba pipeline extension shortly.

Is this still upcoming?

This seems like an easy fix and doesn't seem to interfere with any tests, so could we include it in the numba 0.54 release?

I'm not 100% sure that reassignment ought to be legal, as mentioned in #7258 - @sklam's input may be needed here.

@njriasan
Copy link
Contributor Author

I'll add a reproducer with a Numba pipeline extension shortly.

Is this still upcoming?

Yes. I'll attach it in the next comment.

This seems like an easy fix and doesn't seem to interfere with any tests, so could we include it in the numba 0.54 release?

I'm not 100% sure that reassignment ought to be legal, as mentioned in #7258 - @sklam's input may be needed here.

So from our perspective this is incredibly useful because we have some difficulties with inplace updates. For example, we may need to update an immutable array inplace in a way that changes its length (I can elaborate if necessary). We handle this by updating the reference to where the data is stored, which requires reassignment.

I totally understand if this is not behavior that is needed when just using Numba as a compiler, but since Numba is also a tool for building compilers we believe we have a relevant usecase for why Numba shouldn't complete disallow this behavior. Perhaps there could be a flag used that would allow us to enable this behavior?

@njriasan
Copy link
Contributor Author

njriasan commented Jul 29, 2021

Here is the Python file that can be used to generate IR that produces reassign in a way that vaguely resembles our usecase. I tried to simplify it as much as possible so this isn't a general/complete solution.

Github didn't accept just the .py file so I placed it in a zip file. Sorry if that's annoying.
dummy_reassign_ext.zip

@ehsantn
Copy link
Collaborator

ehsantn commented Jul 29, 2021

Currently, I believe Numba's IR is in SSA form only during typing. This change shouldn't affect SSA IR anyways.

@sklam
Copy link
Member

sklam commented Aug 3, 2021

@njriasan, RE: the reproducer dummy_reassign_ext.zip. Where should I be looking to see the effect of the patch?

UPDATE i can observe it via get_allocation_stats()

Copy link
Member

@sklam sklam left a comment

Choose a reason for hiding this comment

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

Thank you for the patch and the reproducer. It allowed me to find the true problem. See #7267

At every assignment statement in the IR, the old value is always deleted before storing the new value. Therefore, we should never need to have more than 1 del for a variable in a block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants