Enable metrics and fix lock contention #1124
Conversation
Shubham8287
force-pushed
the
shub/fix-lock-metric-contention
branch
from
7c56a1e
to
3b418c1
Compare
Shubham8287
added
test-with-bots
Shubham8287
force-pushed
the
shub/fix-lock-metric-contention
branch
from
3b418c1
to
407fe22
Compare
| tokio::task::spawn_blocking(|| { | ||
| drop(all_ctx); | ||
| }); |
There was a problem hiding this comment.
While this is being done, we're holding a lock on the subscriptions manager. If you return this instead then you can drop the subscriptions lock and do the metrics work after.
There was a problem hiding this comment.
I thought about this, but as we are already not awaiting, it will at max save few microseconds but no strong opinions, can do it.
| @@ -261,6 +265,9 @@ impl SubscriptionManager { | |||
| tracing::warn!(%client.id, "failed to send update message to client: {e}") | |||
| } | |||
| }); | |||
There was a problem hiding this comment.
Not sure how much time we're spending in this eval.into_iter.for_each(...), but we could move this earlier by having the hash map that is iterated on be keyed over the client instead (and making its hash function use only the addr and identity). Then you could move that work to after the subscription lock has been dropped. In any case, this would be for a different PR.
bfops
added
release-any
Shubham8287
force-pushed
the
shub/fix-lock-metric-contention
branch
from
234bf72
to
7c52ef5
Compare
Shubham8287
force-pushed
the
shub/fix-lock-metric-contention
branch
from
8b94df0
to
f5947d3
Compare
|
Bot test failed. Please check the workflow run for details. |
|
Bot test failed. Please check the workflow run for details. |
| @@ -177,6 +177,18 @@ impl From<&ReducerContext> for txdata::Inputs { | |||
| } | |||
| } | |||
|
|
|||
| impl Clone for ExecutionContext { | |||
There was a problem hiding this comment.
A comment for why we need a custom Clone?
There was a problem hiding this comment.
Sure I 'll add,
FYI, it's there so that ctx can be shared across threads without using lock on metrics fields, which might be modified.
cloutiertyler
added
release-0.10
and removed
release-any
bfops
added
release-any
Description of Changes
Made
ctxto not be shared between threads in subscription as that leads to lock contention.API and ABI breaking changes
No
Expected complexity level and risk
2, can affect perfoamance heavily.