Copilot commented on code in PR #13583:
URL: https://github.com/apache/trafficserver/pull/13583#discussion_r3846388764
##########
include/tsutil/Metrics.h:
##########
@@ -354,15 +359,38 @@ class Metrics
current() const
{
std::lock_guard lock(_mutex);
- return {_cur_blob, _cur_off};
+ return {_cur_blob.load(std::memory_order_relaxed),
_cur_off.load(std::memory_order_relaxed)};
}
bool
valid(IdType id) const
{
- auto [blob, entry] = _splitID(id);
+ return _is_allocated(id);
+ }
+
+ private:
+ /** Whether @a id names an allocated slot.
+ *
+ * The gate for every id based accessor, since ids from the @c TSStat* API
are untrusted. An id
+ * qualifies when it is non-negative, its offset is one @c _makeId could
produce, and its slot
+ * has been handed out.
+ */
+ bool
+ _is_allocated(IdType id) const
+ {
+ if (id < 0) {
+ return false;
+ }
+
+ auto [blob_ix, offset] = _splitID(id);
+
+ // _cur_blob first: acquiring it also makes visible everything published
under it.
+ auto const cur_blob = _cur_blob.load(std::memory_order_acquire);
+ auto const cur_off = _cur_off.load(std::memory_order_acquire);
Review Comment:
`Storage::_is_allocated()` unconditionally does an acquire load of
`_cur_off` even though `_cur_off` is only needed when `blob_ix == cur_blob`.
This adds an avoidable acquire barrier on every call (including clearly-invalid
ids and ids for older blobs), which is on a hot path for
`valid()/lookup(id)/name()`.
Consider restructuring to early-return using only `_cur_blob` first, and
only then acquire-load `_cur_off` in the `blob_ix == cur_blob` case.
--
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]
