Reuse context - #259
Conversation
| let ctx = unsafe { proj_context_clone(self.ctx) }; | ||
| crs_to_crs_from_pj(ctx, self, target_crs, area, options) | ||
| let ctx = unsafe { proj_context_clone(self.ctx()) }; | ||
| crs_to_crs_from_pj(Context::Owned(ctx), self, target_crs, area, options) |
There was a problem hiding this comment.
Here create_crs_to_crs_from_pj gets an Owned Context whereas new_known_crs has a Shared Context. Is this difference deliberate?
There was a problem hiding this comment.
It's deliberate: the "derive from an existing Proj" methods (create_crs_to_crs_from_pj etc.) clone self's context, whereas the from-scratch constructors (new, new_known_crs) use the per-thread shared context.
I think the clone originally existed only to avoid a double-free from sharing self's raw context pointer which the ref-counted Shared context now handles anyway. The only remaining reason to clone is to inherit self's context config (search paths / network endpoint / CA bundle) into the derived transformation, which only matters when self came from a configured ProjBuilder.
The downside is that the clone opens a fresh proj.db connection, so this path doesn't get the #256 speedup. If we don't care about config inheritance here I'm happy to switch it to thread_local_context() for consistency and perf; if we do, I'll document it.
There was a problem hiding this comment.
Yeah, the guard against the double free makes sense. Personally I tend to use new_known_crs over create_crs_to_crs_from_pj anyway because I appreciate the axis normalization that happens in new_known_crs. That this path does not get the speedup therefore would not affect me, though I can imagine it is nice have these methods more aligned in that sense. If however that breaks with the ProjBuilder path I understand some caution is warranted, especially if there are people out there count on the difference in context.
|
Some more (hopefully useful) context, since I wanted to make sure that we were doing it the "correct" way now: PROJ's recommended model is one context per thread, shared by all objects created on that thread, rather than one context per object. From the official quickstart:
This is the pattern used by GDAL (a thread-local |
47588ba to
d5a57de
Compare
Proj::new and Proj::new_known_crs now borrow the per-thread shared context instead of creating (and destroying) a fresh PROJ context each call, mirroring pyproj's one-context-per-thread model. This drops Proj::new to ~1us per instance. ProjBuilder and the clone-based constructors continue to own their own contexts. Adds tests for per-thread reuse, per-thread isolation, ProjBuilder context independence, and that dropping a Proj does not free the shared context. Signed-off-by: Stephan Hügel <shugel@tcd.ie>
|
(Need someone to OK this so we can merge) |
michaelkirk
left a comment
michaelkirk
left a comment
There was a problem hiding this comment.
LGTM - I had a question, but it's just for my own understanding.
| //! ## Threading | ||
| //! | ||
| //! Each PROJ context (`PJ_CONTEXT`) is tied to a single thread and must not be used concurrently | ||
| //! from more than one thread. `Proj` is therefore deliberately neither `Send` nor `Sync`; to |
There was a problem hiding this comment.
It seems like it could be Send? But anyway, it's an improvement from what we currently have.
This behavior of using a thread-local is going to be a big speedup for pretty much everybody, but I'm wondering if there is a niche use case where you want to manage context specifically.
With the c-lib you can have multiple contexts per threads (or multiple threads per context if you're careful that they don't overlap access). You can't do that with the rust lib now, right? I think this is a minority (maybe verging on non-existent) use case, but want to make sure I'm understanding.
There was a problem hiding this comment.
I don't think we could make the new design Send because of ProjContext::Shared(Rc<Context>): Rc's ref counts are non-atomic. So sending a Shared to another thread. Would keep a clone of the original Rc (or vice versa?), and that would allow the ref count to be mutated by two threads at once. That's certainly a data race, and might be UB (??).
I think we can do multiple ProjContexts per thread anyway: ProjBuilder keeps its own owned context, and clone_owned() hands a derived Proj an independent context. The thread-local sharing only applies to Proj::new and Proj::new_known_crs. So if you want a dedicated context, you can go through ProjBuilder.
Multiple threads per one context is allowed by The C lib if you serialise access yourself, but our wrapper can't enforce (even before this PR) anything that would make that safe at compile time. Maybe worth thinking about, but I haven't…
There was a problem hiding this comment.
We could improve the docs with e.g. "If you need a context independent of the per-thread one, construct via ProjBuilder"
There was a problem hiding this comment.
That makes sense, thanks for explaining.
3 participants
CHANGES.mdorCHANGELOG.mdBased on #258, so merge that first. This is part 2 of a 2-part stack which improves perf of
Proj::newcreation to around 1us per instance.