[WGPUFuture] Use map over unordered_map for JS event ordering. Bug: dawn:2066 Change-Id: Ie8e720c5e48f3e471a8eb5fe1019bcd0441583ad Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/153000 Reviewed-by: Corentin Wallez <cwallez@chromium.org> Commit-Queue: Loko Kung <lokokung@google.com> Kokoro: Kokoro <noreply+kokoro@google.com>
diff --git a/src/dawn/wire/client/EventManager.cpp b/src/dawn/wire/client/EventManager.cpp index f567078..1e0f1e0 100644 --- a/src/dawn/wire/client/EventManager.cpp +++ b/src/dawn/wire/client/EventManager.cpp
@@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include <unordered_map> +#include <map> #include <utility> #include <vector> @@ -47,13 +47,14 @@ void EventManager::ShutDown() { // Call any outstanding callbacks before destruction. while (true) { - std::unordered_map<FutureID, TrackedEvent> movedEvents; + std::map<FutureID, TrackedEvent> movedEvents; mTrackedEvents.Use([&](auto trackedEvents) { movedEvents = std::move(*trackedEvents); }); if (movedEvents.empty()) { break; } + // Ordering guaranteed because we are using a sorted map. for (auto& [futureID, trackedEvent] : movedEvents) { // Event should be already marked Ready since events are actually driven by // RequestTrackers (at the time of this writing), which all shut down before this. @@ -74,6 +75,7 @@ } void EventManager::ProcessPollEvents() { + // Since events are already stored in an ordered map, this list must already be ordered. std::vector<TrackedEvent> eventsToCompleteNow; // TODO(crbug.com/dawn/2060): EventManager shouldn't bother to track ProcessEvents-type events @@ -120,7 +122,9 @@ return WGPUWaitStatus_Success; } - std::vector<TrackedEvent> eventsToCompleteNow; + // Since the user can specify the FutureIDs in any order, we need to use another ordered map + // here to ensure that the result is ordered for JS event ordering. + std::map<FutureID, TrackedEvent> eventsToCompleteNow; bool anyCompleted = false; const FutureID firstInvalidFutureID = mNextFutureID; mTrackedEvents.Use([&](auto trackedEvents) { @@ -141,7 +145,7 @@ if (event.mReady) { anyCompleted = true; if (event.mCallback) { - eventsToCompleteNow.emplace_back(std::move(event)); + eventsToCompleteNow.emplace(it->first, std::move(event)); } trackedEvents->erase(it); } @@ -149,7 +153,7 @@ }); // TODO(crbug.com/dawn/2066): Guarantee the event ordering from the JS spec. - for (TrackedEvent& event : eventsToCompleteNow) { + for (auto& [_, event] : eventsToCompleteNow) { DAWN_ASSERT(event.mReady && event.mCallback); // .completed has already been set to true (before the callback, per API contract). event.mCallback(EventCompletionType::Ready);
diff --git a/src/dawn/wire/client/EventManager.h b/src/dawn/wire/client/EventManager.h index 2260b73..2c599e2 100644 --- a/src/dawn/wire/client/EventManager.h +++ b/src/dawn/wire/client/EventManager.h
@@ -17,7 +17,7 @@ #include <cstddef> #include <functional> -#include <unordered_map> +#include <map> #include <utility> #include "dawn/common/FutureUtils.h" @@ -70,8 +70,10 @@ Client* mClient; - // Tracks all kinds of events (for both WaitAny and ProcessEvents). - MutexProtected<std::unordered_map<FutureID, TrackedEvent>> mTrackedEvents; + // Tracks all kinds of events (for both WaitAny and ProcessEvents). We use an ordered map so + // that in most cases, event ordering is already implicit when we iterate the map. (Not true for + // WaitAny though because the user could specify the FutureIDs out of order.) + MutexProtected<std::map<FutureID, TrackedEvent>> mTrackedEvents; std::atomic<FutureID> mNextFutureID = 1; };