Source
r/cpp_questions•
tuskcode

How to automatically remove completed std::future<void> from a std::vector<std::future<void>>?

OPEN

I am using futures to execute a messaging system

void RaiseEvent(EventID messageID)
{
	mEventExecutors.push_back(std::move(std::async([=]{
			auto eventObject = mEventListeners.find(messageID);
			if (eventObject != mEventListeners.end())
			{
				for (auto listener : eventObject->second)
				{
					listener();
				}
			}
		})
	));
}

Is there anyway to automate the removal of the completed futures from mEventExecutors? Or do I need to monitor the state of the futures in the vector and manually erase these?

Posted Jul 15, 2017, 8:14 AM UTC

312
sellibitze•

Side note: Your use of std::move is unnecessary. std::move is for casting an lvalue expression into an rvalue expression that refers to the same object. Here, std::async(...) is already an rvalue expression. In some cases an unecessary std::move even makes things worse as it prevents move elision and lifetime extensions of temporaries.

2
SlightlyGrilled•

So this code might not work as I've made it on the fly without compiling or testing, but it should in theory achieve the job.

It works using RAII, and putting the clean up code in the destructor of another class "TaskCleaner", in practice you might want friendship or something like that, but I mocked this up quick. Also it's too late at night for me to fully think through the implications of removing something from the map, while the async is still destructor. Maybe when I'm back home I'll look though it.

class TaskCleaner {
    TaskCleaner(EventManger& eventManger, std::size_t executerID) : 
        mEventManger(eventManger),
        mExecuterID(executerID) 
        {}
    ~TaskCleaner() {
        mEventManger.clean(mExecuterID);
    }
    const std::size_t mExecuterID;
    const EventManger& mEventManger;
}

class EventManger {
public:
    EventManger() {}
    ~EventManger() {}
    void RaiseEvent(EventID messageID)
    {
	    std::lock_guard<std::mutex> guard(mExecutorsMutex);
        mEventExecutors[++mCurrentExecutorID] = std::async([this, mCurrentExecutorID]{
            //when this async destructes this will too, cleaning up this async from the mEventExecutors map.
            TaskCleaner taskCleaner(*this, mCurrentExecutorID);
            auto eventObject = mEventListeners.find(messageID);
            if (eventObject != mEventListeners.end())
            {
                for (auto listener : eventObject->second)
                {
                    listener();
                }
            }
        });
    }
    void CleanExecutor(std::size_t ExecuterID) 
    {
	    std::lock_guard<std::mutex> guard(mExecutorsMutex);
	    mEventExecutors.erase(ExecuterID);
    }
private:
    std::size_t mCurrentExecutorID = { 0 };
    std::map<std::size_t, std::future<void>> mEventExecutors;
    std::mutex mExecutorsMutex;
}

If this doesn't work please let me know and I'll put some further thought in to it.

C++14
If your using c++14 you can use lambda capture initializers
To put the task cleaner in to the capture list instead of inside the async function. not sure if that helps much, but it might allow you to get rid of that copy/this capture.

Infact how are you accessing "mEventListeners" inside that lambda, with only the copy capture, don't you need "this"?

I'll probably update it tomorrow when I'm not really tired.

2
PearlyDewdropsDrops•

Use a std::set or list instead of a vector so you can remove elements without shifting everything after them. There might be a way to make a future remove itself but that would increase the size of each element and make your set non copyable. Consider adding a bool called active and set it false when the future is run. Then leave it there and overwrite when you want to set it again

1
YouFeedTheFish•

Although it's counter-intuitive, this isn't the best advice. Removing things form a vector is often faster due to prefetching and CPU caching.

1
PearlyDewdropsDrops•

it isn't "the best" advice because it sometimes isn't faster ? I think your comment isn't "the best" comment because it doesn't explain when one is faster vs the other. In fact it demonstrates you dont even know when it might be faster. Maybe never ?

1
YouFeedTheFish•

I do not know when it might be faster. You'd have to test. Mileage varies from system to system and also based on other factors, like the number of times insertions or deletions are occurring, the size of the data set, etc. In a standard development environment, I'd hazard to say it is almost always faster to remove an item from a vector than a container relying on non-contiguous memory.

Don't trust me? Take it from the man himself.

1
PearlyDewdropsDrops•

I'm aware of that, but to say it's bad advice is being disingenuous. This isn't play code, OP said it was from a real-life production system. It's possible he's looking up handlers based on an ID, which would be linear time on a vector. Hence why I suggested std::set. To say "that's bad advice because <information you dont have>" is just trashing my explanation without any cause. why dont you actually do the work of figuring that out before you assume ?

0
orbital1337•

Use a std::set or list instead of a vector so you can remove elements without shifting everything after them.

Just use std::swap(vec[i], vec.back()) followed by vec.pop_back() if you don't care about the order. Avoid using datatypes like std::set or std::list unless you really need them which is rarely the case.

1
[deleted]•

[deleted]

1