Skip to content

Conversation

kngwyu
Copy link
Member

@kngwyu kngwyu commented Sep 29, 2018

and use static variable to access Numpy C APIs.
Since nowt we can't ensure operation's safety by PyArrayModule's lifetime, I changed all APIs to return or take &'py PyArray.
TODO:

  • comments
  • notes on README, that indicates really breaking change has done

@kngwyu kngwyu requested a review from termoshtt September 29, 2018 10:52
Copy link
Collaborator

@termoshtt termoshtt left a comment

Choose a reason for hiding this comment

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

Great work 👍
This matches the PyO3 way well.

unsafe {
PyArray_Type_Ptr = mod_.get_type_object(ArrayType::PyArray_Type);
// TODO: this operation is 'mostly safe' because of GIL, but not completely thread safe
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have any plan to fix it?
I try to use lazy_static and Mutex, but it (9bd4376) does not work :<

Copy link
Member Author

@kngwyu kngwyu Sep 30, 2018

Choose a reason for hiding this comment

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

Somewhat calling python ffi from a synchronized path causes dead lock when cargo test executes multiple tests in parallel.
But I think we can ensure safety of substitution to ARRAY_API_CACHE by std::sync::Once

Copy link
Member Author

Choose a reason for hiding this comment

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

i.e.
This is OK

let api = get_numpy_api(MOD_NAME, CAPSULE_NAME);
INIT_API.call_once(move || {
    ARRAY_API_CACHE = PyArrayAPI_Inner(api);
});

but this causes dead lock, in parallel tests

INIT_API.call_once(move || {
    let api = get_numpy_api(MOD_NAME, CAPSULE_NAME);
    ARRAY_API_CACHE = PyArrayAPI_Inner(api);
});

Copy link
Member Author

Choose a reason for hiding this comment

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

And I think we don't have to use Mutex.
We have to lock the access to Numpy API only once in a program, since Numpy API is pinned by capsule.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems to be correct.

@kngwyu
Copy link
Member Author

kngwyu commented Sep 30, 2018

Thanks for review, I synchronized the substitution to static.
But, still python ffis can be called from multi threads(though they have their own lock, I think).

@termoshtt
Copy link
Collaborator

But, still python ffis can be called from multi threads(though they have their own lock, I think)

The parallel test by cargo test calls APIs in parallel, but it does not cause any problem. GIL works fine for this.

Copy link
Collaborator

@termoshtt termoshtt left a comment

Choose a reason for hiding this comment

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

LGTM

@kngwyu kngwyu merged commit 8c0f427 into PyO3:master Sep 30, 2018
@kngwyu kngwyu deleted the remove-pyarraymodule branch September 30, 2018 07:17
@kngwyu kngwyu mentioned this pull request Sep 30, 2018
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants