Best way to keep a collection of timers (object vs pointer)

Question

I've been trying to implement a wrapper class for chrono's high_performance_clock. Basically there is a Diagnostics mother class that keeps a container of TaskTimers. Up until now I have been keeping them as raw pointers in a std::vector<TaskTimer*> list, but reading up on smart pointers and pointer vs object discussions, I'm not sure what to think. It seems results can be in favor or against either, depending on the experiment.

Is there a general consensus of what's considered 'correct' in this case? I'm looking for performance rather than readability.

The Diagnostics class allows the programmer to place timers in any place of the code, as well as start, pause and stop them. They can be accessed by name(std::string) or id(int).

std::unique_ptr<std::vector<std::shared_ptr<TaskTimer>>> m_TimerList;

Snippet from Diagnostics:

inline int AddTimer(const std::string& timerName, bool runImmediately = true) {
        if (IsEnabled()) {
            auto it = std::find_if(m_TimerList->begin(), m_TimerList->end(), [timerName](TaskTimer* obj) { return obj->GetName() == timerName; });
            if (it != m_TimerList->end()) { return -1; } // exists
            else {
                std::shared_ptr<TaskTimer> t(new TaskTimer(timerName, m_NextID, m_Debug));
                m_TimerList->push_back(t);
                if (runImmediately) { _Start(t); }
                return m_NextID++;
            }
            return -1;
        }
        return -2;
    }

inline void Start(const int timerID) {
    auto el = _GetFromID(timerID);
    if (el != nullptr) { _Start(el); }
}

inline void Pause(const std::string& timerName) {
    if (!IsEnabled()) { return; }
    auto el = _GetFromName(timerName);
    if (el != nullptr) { el->Pause(); }
}

inline std::shared_ptr<TaskTimer> _GetFromID(const int id) const {
    const auto it = std::find_if(m_TimerList->begin(), m_TimerList->end(), [&id](std::shared_ptr<TaskTimer>& obj) {return obj->GetID() == id; });
    if (it != m_TimerList->end()) { return (*it); }
    return nullptr;
}

inline std::shared_ptr<TaskTimer> _GetFromName(const std::string& name) const {
    const auto it = std::find_if(m_TimerList->begin(), m_TimerList->end(), [&name](std::shared_ptr<TaskTimer>& obj) {return obj->GetName() == name; });
    if (it != m_TimerList->end()) { return (*it); }
    return nullptr;
}

inline void _Start(std::shared_ptr<TaskTimer> t) {
    if (!t->IsStarted()) {
        t->Start();
        m_StartedCount++;
    }
    else {
        if (!t->IsRunning()) { t->Start(); }
        else                 { t->Resume(); }
    }
}

I'd like to understand the reasoning behind what is the 'correct' choice here in terms of object/pointer management. Is my collection of TaskTimers optimal for performance as it is now? Is there a better way to do it while retaining the current functionality?

stack overflow isn't a sight for soliciting opinions. It is for specific technical questions about programming. Phrases like "better" and "right" are not objective measures. — xaxxon, Aug 23 '18 at 04:13
There is almost never a reason to have a dynamically allocated `std::vector`. Just use the vector straight. Is there a special reason you are storing `std::shared_ptr` rather than just `TaskTimer` in your vector? — Galik, Aug 23 '18 at 06:06
@Galik Thanks for making me realize they don't need to be dynamically allocated. — Mads Midtlyng, Aug 23 '18 at 09:53

Jeremy Friesner · Accepted Answer · 2018-08-23T05:09:01.550

Raw pointers are almost always the wrong choice for dynamically-allocated objects, as they inevitably lead to memory-leaks and/or use-after-free-errors.

So the only real decision is smart-pointer vs value-semantics; the answer to that will depend a lot on how your TaskTimer class is implemented.

Is a TaskTimer a "lightweight" or "POD" object that is very cheap and simple to make copies of? If so, then simply holding TaskTimer objects by-value (e.g. std::vector<TaskTimer>) is fine.
OTOH, is it computationally expensive (or even a compile-time or run-time error) to make a copy of a TaskTimer object? If so, then smart-pointers are the way to go, since that approach will allow your TaskTimer objects to have lifetimes longer than that of the scope they were created in.

Thank you for clearing up how I should regard this. In my case, the TaskTimer is a lightweight timer composed of pure C++ and POD types. I realize I don't need to dynamically allocate them now :) — Mads Midtlyng, Aug 23 '18 at 09:52

Best way to keep a collection of timers (object vs pointer)

1 Answers1