neondatabase / neon

Neon: Serverless Postgres. We separated storage and compute to offer autoscaling, code-like database branching, and scale to zero.
https://neon.tech
Apache License 2.0
14.78k stars 430 forks source link

CancellationToken locks a std::sync::Mutex each time `is_cancelled` is called #5834

Closed jcsp closed 6 months ago

jcsp commented 11 months ago

We need this to be cheap so that when we're e.g. in a loop of I/Os, we can check for cancellation at each iteration without forcing a spurious synchronization between every task that is checking the same cancellation token.

This can probably be fixed quite quickly by adding an atomic that we can check without taking the lock. In the contexts where we use this, we are usually doing a best-effort check, so a relaxed read would work.

We can also consider how much we really need the tree hierarchy that CancellationToken offers. Since we already do quite organized shutdown (shutting down timelines during tenant shutdown, etc), we don't use a lot of parent-child token relationships. If we decide we don't need the tree behavior, then a much simpler impl might do the job.

koivunej commented 11 months ago

I disagree with this being a problem if we created per operation subtokens then the mutex would not be contented and cost would be at the cancellation site. I think that's an easy solution. Any atomic would still incur cost.

I do think we need the hierarchy support; working out non-hierarchical tokens manually does not seem sensible to me but only create more troubles, namely code we need to maintain.

Related thread: https://neondb.slack.com/archives/C033RQ5SPDH/p1699463862210049