[SYCL] [L0] Fix for event leakage when using immediate commandlists. #7546
Conversation
| EventList.clear(); | ||
| EventList = std::move(UnsetEventsList); |
There was a problem hiding this comment.
seems like some redundant copying, why do you need a new UnsetEventsList at all?
There was a problem hiding this comment.
Trying to delete elements from EventList inside a loop which is iterating over EventList does not work. Odd things happen, like skipping elements in the original EventList. That caused the leak. It is safer to iterate over EventList, gather remaining elements in another list and then update EventList.
There was a problem hiding this comment.
Trying to delete elements from EventList inside a loop which is iterating over EventList does not work. Odd things happen, like skipping elements in the original EventList. That caused the leak.
Should the fix is be as simple as updating the iterator as you erase:
it = EventList.erase(it);
There was a problem hiding this comment.
Or maybe std::remove_if(begin, end, predicate/lambda) ? Or std::list::remove_if if that's the container being used here.
There was a problem hiding this comment.
A combination of the suggestions was used. The additional list has been eliminated and deletions from EventList are done in-place.
| std::move(it, it, std::back_inserter(EventListToCleanup)); | ||
| EventList.erase(it, it); | ||
| EventListToCleanup.push_back(std::move((*it))); | ||
| it = EventList.erase(it, it); |
There was a problem hiding this comment.
What if it was the last event and it now points to end iterator? Wouldn't it++ of it fail?
There was a problem hiding this comment.
Even a removal at the end of the list works as expected.
There was a problem hiding this comment.
My question wasn't about the removal but about the it++ executed along the loop backedge after said removal is done. Is that ok as well?
There was a problem hiding this comment.
It was not OK to simply update the iterator.
Further digging revealed why in an earlier version of the code some events in the incoming list were skipped during examining for being reset. Now, the for-loop does not increment the iterator. If an event is removed the iterator is set to the element after the one being removed, and if the event is not erased then the iterator is incremented. This avoid the double ++ that was occurring before, whenever an event was removed.
|
SYCL :: AtomicRef/max_local.cpp is being tracked in #7604 |
This change properly gathers events that have been set and need to be recycled.