Don't attach to runtime when cloning TaskLocals.

[iksteen]

As discussed in #61. Attaching to the runtime isn't free but it happens every time a TaskLocals instance is cloned which happens for pretty much any future_into_py call.

This was introduced because the old method, directly cloning the python reference without holding the GIL, wasn't sound as described in https://pyo3.rs/main/migration.html#pyclone-is-now-gated-behind-the-py-clone-feature.

While acquiring the GIL is the neater option: all reference counting is handling within the python runtime, it's not strictly necessary. The migration guide mentions using Arc<Py<T>> which allows us to reference count the python reference itself on the Rust side. Cloning an Arc is very cheap so performance gets a boost.

The only downside is that reasoning about the reference count of a python object becomes more complicated: there's a reference count on both the python object and the Rust side. Fortunately, there is at most one Arc per python reference.

Since cloning TaskLocals now no longer needs a python runtime, the cloning operation can become an implementation of the Clone trait.

[kylebarron]

I don't have merge/publish access, so I don't make the end decision, but one thing I was wondering: usually this crate is just released whenever there's a breaking pyo3 release. (I think it would be nice to keep the package version here in sync with the package version of pyo3).

But if you want a release before then, I think it would help to make this PR non-breaking. We could keep this clone_ref method as a dummy that just calls self.clone(), add a #[deprecated], and remove it in the next breaking release. (I'm not sure if it's ok to add #[deprecated] in a patch release?). Then we can create an issue to make sure we actually remove it in the next breaking release.

[iksteen]

Ah, good call. I mistakenly assumed this was mostly an internal method.

I'll add the wrapper and also mark that deprecated. SemVer says that deprecation only 'requires' a minor version bump if the major version >= 1.

[kylebarron]

I don't know who other than @davidhewitt has merge access to this repo

[davidhewitt]

Sorry I have been travelling so haven't had a chance to look at all of the above properly, @kylebarron I've sent you an invite to have write access 👍

[kylebarron]

No worries, and thanks! @davidhewitt in general should we wait for 2 approvals before merging?

[davidhewitt]

I am happy for you to proceed without waiting for me, though I hope to give useful input to the library when I can (and also to PyO3's experimental async support which can hopefully enhance the stuff going on here one day).

[iksteen]

I'm in no particular rush btw, I'm building my library off this branch by using a git submodule (and it's happily purring away in production). Like I said in my initial issue, I'm not extremely proficient in Rust. I'd rather wait a bit so everyone can take a good think and look than hurriedly merge something that doesn't quite work all the time.

Oh, and you're all doing a phenomenal job responding quickly!

[stepancheg]

Fixing tokio getting blocked by GIL is critical in our use case, so if it is possible to land this PR without too much delay, I'd appreciate it.

The PR looks good (provided I have little experience working with pyo3), but it can be made slightly more efficient if

pub struct TaskLocals {
    event_loop: Arc<Py<PyAny>>,
    context: Arc<Py<PyAny>>,
}

is replaced with

struct TaskLocalsInner {
    event_loop: Py<PyAny>,
    context: Py<PyAny>,
}

pub struct TaskLocals(Arc<TaskLocalsInner>);

[iksteen]

Excellent suggestion @stepancheg. Initially I thought this wasn't feasible because of the with_context method that needs to make a copy of event_loop for a new instance of TaskLocals. But since context is bound there we can create a new ref to event_loop on the python side.

[stepancheg]

Let's merge this PR?