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

Unify GILGuard and Python #213

Closed
konstin opened this issue Sep 2, 2018 · 7 comments
Closed

Unify GILGuard and Python #213

konstin opened this issue Sep 2, 2018 · 7 comments

Comments

@konstin
Copy link
Member

konstin commented Sep 2, 2018

Currently, there a two types: GILGuard and Python, where GILGuard carries data and has a Drop implementation, while Python is a zero sized marked type. This leads to the following boilerplate all over the code:

let gil = Python::acquire_gil();
let py = gil.python();

We should unify those two types, so that we can just write

let gil = GIL::acquire();

and then use the gil just like the py now.

@fafhrd91
Copy link
Contributor

fafhrd91 commented Sep 2, 2018

I don’t think this is possible. Also acquire is rare for extensions

@joar
Copy link
Contributor

joar commented Sep 11, 2018

Just to add a datapoint, I use acquire quite a lot in https://github.com/joar/rust-csv-py

@kngwyu
Copy link
Member

kngwyu commented Oct 6, 2018

@joar
Since acquiring gil can be overhead(though it's not so large), I recommend to use self.token.py() when it's possible(e.g. here).

@kngwyu
Copy link
Member

kngwyu commented Oct 6, 2018

If we remove Python<'p>(PhantomData<&'p GILGuard>), then we have to pass &'py GILGuard to functions, instead of Python<'p>.
So... what different does it make?
And, how about PyToken?
I have no idea to replace it with &GILGuard, without lifetime problem.

@konstin
Copy link
Member Author

konstin commented Oct 6, 2018

I'd rather like to remove PyToken completely (see #94).

For the GILGuard vs. Python story I thought about using things like Deref or a Python type that hides the GILGuard internally, but I haven't found a good design yet.

@kngwyu
Copy link
Member

kngwyu commented Oct 6, 2018

I see.
Yeah I feel like PyToken is not 'the best' approach, but have no better ides now 🤔

@konstin konstin mentioned this issue Nov 12, 2018
joar added a commit to joar/rust-csv-py that referenced this issue Nov 14, 2018
@davidhewitt
Copy link
Member

I think that most users will use the new Python::with_gil API, so I'm not sure that any changes here are really necessary.

As a result I'm going to close this issue, but please shout if anyone thinks this still needs to happen and we can reopen.

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

No branches or pull requests

5 participants