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

Allocate new memory in the stack to save array transpose value insteading of sharing memory with original array, this fix numba#6949 . #7011

Closed
wants to merge 2 commits into from

Conversation

guoqiangqi
Copy link
Contributor

@guoqiangqi guoqiangqi commented May 10, 2021

The result of array inpalce binop(such as+=`) is ircorrect since the array transpose data are sharing memory with original array, which cased the transposed array data changed during the inplace binop calculation. For example (provided in #6949):

@jit 
def self_addition(x):
    x += x.T
    return x

x = np.array([[1,2],[3,4]])

self_addition(x)
> array([[2, 5],[8, 8]])

In this case, x will be temporarily changed to [[2,5], [3,4]] since the first row will be calculated first when matrix addition is performed. Therefore, x.T will be [[2,3], [5,4]] (the correct value is [[1,3], [2,4]]) when the second row is calculated. Therefore, numba should allocate new memory in the stack to save array transpose data insteading of sharing memory with original array.

@guoqiangqi guoqiangqi marked this pull request as draft May 10, 2021 09:15
@guoqiangqi guoqiangqi changed the title Add the check of the 'noalias' attrubute before lowering 'inplace_bin' op, this fix #6949 . [WIP]Add the check of the 'noalias' attrubute before lowering 'inplace_bin' op, this fix #6949 . May 10, 2021
@guoqiangqi guoqiangqi changed the title [WIP]Add the check of the 'noalias' attrubute before lowering 'inplace_bin' op, this fix #6949 . WIP: Add the check of the 'noalias' attrubute before lowering 'inplace_bin' op, this fix #6949 . May 10, 2021
…ding of sharing memory with original array, this fix numba#6949 .
@guoqiangqi guoqiangqi marked this pull request as ready for review May 19, 2021 09:10
@guoqiangqi guoqiangqi changed the title WIP: Add the check of the 'noalias' attrubute before lowering 'inplace_bin' op, this fix #6949 . Allocate new memory in the stack to save array transpose value insteading of sharing memory with original array, this fix numba#6949 . May 19, 2021
Copy link
Member

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

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

Many thanks for the PR! Unfortunately I think there are a couple of issues with the approach.

Allocating space on the stack works for small data, but larger data exceeds the available stack. For example:

from numba import njit
import numpy as np

x = np.zeros((4096, 4096))


@njit
def self_addition(x):
    x += x.T
    return x


print(self_addition(x))

segfaults:

$ python repro.py 
Segmentation fault (core dumped)

Even for small data, returning a pointer to stack-allocated data is not safe. For example:

from numba import njit
import numpy as np

x = np.array([[1, 2], [3, 4]])


@njit
def get_transpose(x):
    xT = x.T
    return xT


xT = get_transpose(x)

print(xT)

gives:

$ python dangling_pointer.py 
[[140688323904944 140688323927200]
 [ 94515098062032               0]]

(the values are different every time because something else will overwrite the memory pointed to).

In general I think it will be a performance regression to always copy data to create a transpose - ideally a copy should only be made when data overlaps. If it is hard to determine when there is an overlap, it may be that inplace binary operations need to store their result into a temporary array until all results have been computed. It may be worth looking at how NumPy handles the operation x += x.T.

@gmarkall gmarkall added 4 - Waiting on author Waiting for author to respond to review and removed 2 - In Progress labels May 19, 2021
@guoqiangqi
Copy link
Contributor Author

@gmarkall Thanks for your advices, i would try to find a better implementation for this. I will ping you again when ready.

@guoqiangqi
Copy link
Contributor Author

Closed this PR since i have not found a good implementation to fix the noticed issue, anyone can feel free to proposal a new PR if they find a correct solution.

@guoqiangqi guoqiangqi closed this May 20, 2021
@gmarkall
Copy link
Member

Many thanks for your efforts so far!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Waiting on author Waiting for author to respond to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants