Initial stats cleanup

[nirandaperera]

Statistics was constructible directly (make_shared<Statistics>(false) was not equal to Statistics::disabled()), MemoryRecorder held a raw Statistics* (lifetime hazard noted by an existing TODO), provider statistics() accessors returned shared_ptr by value (atomic refcount bumps on hot paths), and secondary objects like AllGather redundantly carried their own statistics_ member already reachable through the buffer resource / context.

In this PR, following changes were made

• Make Statistics constructible only through factory methods (create(), from_options(), disabled()); the public ctor is removed and disabled() is now the canonical singleton for the disabled case.
• Inherit Statistics from std::enable_shared_from_this so MemoryRecorder owns a shared_ptr<Statistics> instead of a raw pointer; recorder no-ops when stats are disabled.
• Introduce a StatisticsProvider concept and have providers (BufferResource, ProgressThread, streaming::Context) return std::shared_ptr<Statistics> const& from statistics(); each provider static_asserts the concept.
• Drop the redundant statistics parameter and statistics_ member from coll::AllGather (and its Python binding); secondary classes now derive their Statistics from the provider they already receive.

Related to #1008

Depends on #1003

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[wence-]

Committed in error?

[nirandaperera]

@wence- this is a draft on top of #1003. Let's merge it first, then I will rebase this.

[nirandaperera]

@wence- this is ready for review now

[pentschev]

At a high-level this looks ok. Left some minor suggestions.

[pentschev]

This looks good to me, thanks Niranda.

[wence-]

I think we need to think through what this design, especially around disabled statistics, looks like. Right now it seems very over-complicated and has, from my reading, somewhat inconsistent semantics.

Before writing some more code, let us figure out what it is we actually want to support (it can be different from what we support today!)

[wence-]

I've had another read over this and I think it is in a good place now.

Minor nits to fix in this round before merging: where you introduce new construction arguments that provide a default value for statistics (e.g. from_options) please just require a statistics object (because of planned followup to remove statistics defaults).

Also, I think it is better to return shared_ptr const& from statistics providers. The consumer can then either get a shared_ptr or just use the method slot directly.

[wence-]

So I think there was some back and forth here. TBH, I think that the statistics method should return shared_ptr<Statistics> const&.

That way, the following works "as expected" but everything is efficient.

auto br = std::make_shared<BufferResource>(...);

br->statistics()->record(); // doesn't bump refcount

auto stats = br->statistics(); // does bump refcount, because stats is a shared_ptr, not shared_ptr&

[nirandaperera]

I was thinking on the same lines here, but then decided against it, because IMO returning the value forces callers to be mindful of the lifetime aspects (and at minimum its the safest).

[pentschev]

One minor docstring issue, otherwise LGTM. Thanks Niranda.

[nirandaperera]

/merge