moonchen commented on code in PR #13583:
URL: https://github.com/apache/trafficserver/pull/13583#discussion_r3928496572
##########
src/tsutil/Metrics.cc:
##########
@@ -179,63 +190,21 @@ Metrics::Storage::type(IdType id) const
return _extractType(id);
}
-Metrics::SpanType
-Metrics::Storage::createSpan(size_t size, Metrics::MetricType type,
Metrics::IdType *id)
-{
- release_assert(size <= MAX_SIZE);
- std::lock_guard lock(_mutex);
-
- // On the final blob there is nowhere left to grow, so refuse a span that
would fill or overflow
- // it rather than letting addBlob() assert. Same intent as the guard in
create(), and the same
- // cost: some slots of the last blob go unused.
- if (_cur_blob >= MAX_BLOBS - 1 && _cur_off + size >= MAX_SIZE) {
- if (id) {
- *id = 0; // Slot 0 is the reserved bad_id.
- }
- return {};
- }
-
- // A span has to be contiguous, so one that does not fit in the current blob
starts a new one.
- if (_cur_off + size > MAX_SIZE) {
- addBlob();
- }
-
- Metrics::IdType span_start = _makeId(_cur_blob, _cur_off, type);
- Metrics::NamesAndAtomics *blob = _blobs[_cur_blob].get();
- Metrics::AtomicStorage &atomics = std::get<1>(*blob);
- Metrics::SpanType span = Metrics::SpanType(&atomics[_cur_off],
size);
-
- if (id) {
- *id = span_start;
- }
-
- _cur_off += size;
-
- // create() grows as soon as it consumes the last slot; do the same here.
Otherwise a span ending
- // exactly on the boundary leaves _cur_off at MAX_SIZE, and the next
create() writes one past the
- // end of the blob's name array. It also makes end() unreachable for
iterator::next(), which
- // wraps on ++offset == MAX_SIZE.
- if (_cur_off >= MAX_SIZE) {
- addBlob();
- }
-
- return span;
-}
-
bool
Metrics::Storage::rename(Metrics::IdType id, std::string_view name)
{
- auto [blob_ix, offset] = _splitID(id);
- Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get();
-
// We can only rename Metrics that are already allocated
- if (!blob || (blob_ix == _cur_blob && offset > _cur_off)) {
+ if (!_is_allocated(id)) {
return false;
}
- std::string &cur = std::get<0>(std::get<0>(*blob)[offset]);
+ // The name is the key _lookups is indexed by, so the whole replacement is
serialized.
std::lock_guard lock(_mutex);
Review Comment:
The acquire on `_next_free` only publishes a slot's initial construction.
This mutex protects the writer, but `lookup(id, &out_name)` and `name(id)`
still read the same `std::string` without `_mutex`, and both expose a
`string_view` into it. I reproduced a TSAN race between the assignment below
and the read in `lookup()` at line 136. Please give metric names stable
immutable storage with suitable synchronization/lifetime semantics, or remove
`rename()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
