WT-12435 Add statistic that tracks how many API calls are in flight #10514
Conversation
|
There is an existing patch(es) for this commit SHA: Please note that the status that is posted is not in the context of this PR but rather the (latest) existing patch and that may affect some tests that may depend on the particular PR. If your tests do not rely on any PR-specific values (like base or head branch name) then your tests will report the same status. If you would like a patch to run in the context of this PR and abort the other(s), comment 'evergreen retry'. |
src/include/api.h
Outdated
| if (!WT_SESSION_IS_DEFAULT(s)) { \ | ||
| uint64_t _api_count_in, _api_count_out; \ | ||
| WT_READ_ONCE(_api_count_out, S2C(s)->counter##_out); \ | ||
| WT_READ_ONCE(_api_count_in, S2C(s)->counter##_in); \ |
There was a problem hiding this comment.
This check is failing only on ARM and PPC - I need to figure out if they need a different primitive on those platforms that WT_READ_ONCE.
I suspect I need to use WT_ACQUIRE_READ instead of WT_READ_ONCE. I'll give that a try.
src/include/api.h
Outdated
| #define WT_API_COUNTER_REALIZE(s, counter, output) \ | ||
| { \ | ||
| uint64_t _api_count_in, _api_count_out; \ | ||
| WT_ACQUIRE_READ(_api_count_out, S2C(s)->counter##_out); \ |
There was a problem hiding this comment.
@luke-pearson - this is the code we were discussing in Slack.
I'm confident the WT_ACQUIRE_READ is safe enough for what I want - let me know if it would be better to use a different mechanism.
There was a problem hiding this comment.
Well this is certainly a case that we haven't yet got a guideline for. I'd say a matching atomic load is still better as this doesn't pair with a RELEASE_WRITE. That is, if you write atomically, read atomically. Still SEQ_CST is a heavy hammer but we don't have much else. If it's not performant I'd think then there needs to be a bigger conversation. You'd need to implement a SEQ_CST load macro.
There was a problem hiding this comment.
I agree with Luke that it would be nice to pair ACQUIRE_READs with RELEASE_WRITEs. This is a constraint I'd like to add in the WT-10967 documentation task to make it easier for developers to reason about the code.
We could do
__wt_atomic_load64(&S2C(s)->counter##_out);
/* Make sure we read counter_out before counter_in */
WT_ACQUIRE_BARRIER;
__wt_atomic_load64(&S2C(s)->counter##_in);
which gives us relaxed loads for the atomicity and still maintains ordering.
I need to read through the code changes and prior PR comments, but stepping back for a second I have some general questions:
API_COUNTER_REALIZEis only used in the stat collection thread. Do we need to worry about performance impacts? The stat increments in the API entry/exit are more impactful.- Why are we using separate
_api_count_inand_api_count_outcounters and then deriving the count? A singleapi_countthat gets decremented on API exit would do away with any ordering concerns.
| /* | ||
| * Helper macros to correctly read and check the API counters. Carefully read the out counter before | ||
| * the in counter otherwise the out counter can include more API calls than the in and make the | ||
| * balance negative. |
There was a problem hiding this comment.
I'd suggest that the comment should also mention that this requires in and out counters use sequential consistency (seq-cst) memory ordering.
No description provided.