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?

[wence-]

question: What does having a create static method buy us over a constructor?

[nirandaperera]

factory methods allows us to safely use std::enable_shared_from_this, where we can pass a shared_ptr to memory recorder rather than a ptr

[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 is OK as you're going for a hot-path optimization, but the lifetime contract should be explicit. Otherwise returning shared_ptr by value is the safer and more idiomatic API.

[nirandaperera]

@pentschev I was addressing @wence- 's earlier suggestion.

I am more inclined to use const ref, because I think the StatisticsProvider in the scope will likely keep the stats alive. Ex:

void bar(const foo& f){ // foo is a Stats provider
// multiple statistics calls 
f->statistics()->add_bytes_stat(..);
f->statistics()->add_duration_stat(..);
}

So, here, unless a temp stats copy is kept in bar scope, we will be creating two unnecessary copies. We dont really need a copy anyway, because f is keeping stats alive.
Now, copy will definitely a must, if statistics were a class variable. Ex: MemoryRecorder.

[pentschev]

At a minimum you should make the contract explicit in the docs, it is one of those things that are easy to overlook when you're just glancing at the return type and think "this returns a shared_ptr, so I can manage it's lifetime have a copy while I keep it alive".

[nirandaperera]

@pentschev After thinking about this a bit more, I reverted stats to return by value.

[pentschev]

"and owning this object" doesn't make sense here, std::shared_ptr<Statistics> is passed by value.

[pentschev]

Suggested change

* Current providers: `BufferResource`, `ProgressThread`, `streaming::Context`.

[pentschev]

Probably best not to document providers here as this will eventually fall out-of-sync.

[pentschev]

This looks good to me, thanks Niranda.