[PATCH] D43901: Make STATISTIC() values available programmatically

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

Thanks

In https://reviews.llvm.org/D43901#1024928, @rtereshin wrote:

> LGTM
>
> My only concern here is that I don't quite understand why only the `ManagedStatic<StatisticInfo> StatInfo`'s writer is guarded by a mutex, but non of the readers. As the newly added `GetStatistics()` method leaks a read-access to `StatInfo` into the wild losing any control over it, it might be a problem.
>
> Could you please clarify that a little bit?


The value itself isn't protected by a mutex but rather ensures atomic updates with std::atomic. Do you mean the StatLock mutex referenced by RegisterStatistic()? That's protecting against a race that can occur on the first modification to the value. The StatisticInfo object records a list of all Statistic objects that were modified at some point. The first time Statistic modified it registers itself with the StatisticInfo object and we need a lock to avoid double-registration (or worse).

> Ah, yeah, also, as the range returned is a couple of vector iterators, it looks like bumping any counter anywhere would potentially invalidate all the ranges existing. I suspect that might be a little error-prone and fragile even in a single-threaded environment. Maybe replace that vector with a list? And add a test that will check we don't regress on this little detail.
>  Alternatively, we could just copy the whole thing instead of returning iterators to it. Potentially protect that copy with that mutex and then we're rock solid.
>  But again, why do we even worry about this being thread-safe in any way one more time?

For a lot of tools, returning iterators is going to be fine. The process would typically be:

- compile, possibly multiple compilations on multiple threads
- wait for compilations
- extract statistics

That's certainly good enough for my aims. However, you're right that there's cases where races can happen. For the most part, they're not an issue since to use this as a measuring tool you already need to manage your threads such that you don't have noise in the statistics from things you didn't want to measure. With those requirements, there's no new registrations while the results are retrieved. However, if it's used to capture a snapshot of the current values then stopping the world is a burden and it's possible for a new statistic to be registered while you're reading them, invalidating the iterators.

I'll switch it to return a std::vector<std::pair<StringRef(), unsigned>>. That will cover the latter use case too


Repository:
  rL LLVM

https://reviews.llvm.org/D43901