Reland "Add a per-thread proc table using thread local storage"

This is a reland of b04a92f01be0e7dc2faf91e0f6f974f990e8b2f2
with the deletion of a duplicate exported function in dawn_wire that
was causing a compilation failure on Windows.

Original change's description:
> Add a per-thread proc table using thread local storage
>
> In situations where both dawn_wire and dawn_native are used on separate
> threads (Chrome with --single-process or --in-process-gpu), it's
> desirable to have a per-thread proc table so that the WebGPU C++ API can
> still be used. This eliminates classes of bugs with manual
> reference/release errors.
>
> This also changes many of the GetProcs functions to return const
> references to the static proc tables known at compile time, instead of a
> copy.
>
> Bug: none
> Change-Id: I8775bb715b312dd9476a1903fbd797d4b1302614
> Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/29240
> Reviewed-by: Stephen White <senorblanco@chromium.org>
> Reviewed-by: Corentin Wallez <cwallez@chromium.org>
> Commit-Queue: Austin Eng <enga@chromium.org>

Bug: none
Change-Id: Id90e5372132cd93a2f8631c8185d0e71b01bc1af
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/29443
Commit-Queue: Austin Eng <enga@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Stephen White <senorblanco@chromium.org>

generator/dawn_json_generator.py

@@ -687,6 +687,10 @@ class MultiGeneratorFromDawnJSON(Generator):
             renders.append(
                 FileRender('dawn_proc.c', 'src/dawn/dawn_proc.c',
                            [base_params, api_params]))
+            renders.append(
+                FileRender('dawn_thread_dispatch_proc.cpp',
+                           'src/dawn/dawn_thread_dispatch_proc.cpp',
+                           [base_params, api_params]))
 
         if 'dawncpp' in targets:
             renders.append(

generator/templates/dawn_native/ProcTable.cpp

@@ -135,16 +135,17 @@ namespace dawn_native {
         return result;
     }
 
-    DawnProcTable GetProcsAutogen() {
-        DawnProcTable table;
-        table.getProcAddress = NativeGetProcAddress;
-        table.createInstance = NativeCreateInstance;
+    static DawnProcTable gProcTable = {
+        NativeGetProcAddress,
+        NativeCreateInstance,
         {% for type in by_category["object"] %}
             {% for method in c_methods(type) %}
-                table.{{as_varName(type.name, method.name)}} = Native{{as_MethodSuffix(type.name, method.name)}};
+                Native{{as_MethodSuffix(type.name, method.name)}},
             {% endfor %}
         {% endfor %}
-        return table;
-    }
+    };
 
+    const DawnProcTable& GetProcsAutogen() {
+        return gProcTable;
+    }
 }

generator/templates/dawn_proc_table.h

@@ -17,6 +17,7 @@
 
 #include "dawn/webgpu.h"
 
+// Note: Often allocated as a static global. Do not add a complex constructor.
 typedef struct DawnProcTable {
     WGPUProcGetProcAddress getProcAddress;
     WGPUProcCreateInstance createInstance;

generator/templates/dawn_thread_dispatch_proc.cpp

@@ -0,0 +1,52 @@
+#include "dawn/dawn_thread_dispatch_proc.h"
+
+#include <thread>
+
+static DawnProcTable nullProcs;
+thread_local DawnProcTable perThreadProcs;
+
+void dawnProcSetPerThreadProcs(const DawnProcTable* procs) {
+    if (procs) {
+        perThreadProcs = *procs;
+    } else {
+        perThreadProcs = nullProcs;
+    }
+}
+
+static WGPUProc ThreadDispatchGetProcAddress(WGPUDevice device, const char* procName) {
+    return perThreadProcs.getProcAddress(device, procName);
+}
+
+static WGPUInstance ThreadDispatchCreateInstance(WGPUInstanceDescriptor const * descriptor) {
+    return perThreadProcs.createInstance(descriptor);
+}
+
+{% for type in by_category["object"] %}
+    {% for method in c_methods(type) %}
+        static {{as_cType(method.return_type.name)}} ThreadDispatch{{as_MethodSuffix(type.name, method.name)}}(
+            {{-as_cType(type.name)}} {{as_varName(type.name)}}
+            {%- for arg in method.arguments -%}
+                , {{as_annotated_cType(arg)}}
+            {%- endfor -%}
+        ) {
+            {% if method.return_type.name.canonical_case() != "void" %}return {% endif %}
+            perThreadProcs.{{as_varName(type.name, method.name)}}({{as_varName(type.name)}}
+                {%- for arg in method.arguments -%}
+                    , {{as_varName(arg.name)}}
+                {%- endfor -%}
+            );
+        }
+    {% endfor %}
+{% endfor %}
+
+extern "C" {
+    DawnProcTable dawnThreadDispatchProcTable = {
+        ThreadDispatchGetProcAddress,
+        ThreadDispatchCreateInstance,
+{% for type in by_category["object"] %}
+    {% for method in c_methods(type) %}
+        ThreadDispatch{{as_MethodSuffix(type.name, method.name)}},
+    {% endfor %}
+{% endfor %}
+    };
+}

generator/templates/dawn_wire/client/ApiProcs.cpp

@@ -293,21 +293,16 @@ namespace dawn_wire { namespace client {
         return result;
     }
 
-    //* Some commands don't have a custom wire format, but need to be handled manually to update
-    //* some client-side state tracking. For these we have two functions:
-    //*  - An autogenerated Client{{suffix}} method that sends the command on the wire
-    //*  - A manual ProxyClient{{suffix}} method that will be inserted in the proctable instead of
-    //*    the autogenerated one, and that will have to call Client{{suffix}}
-    DawnProcTable GetProcs() {
-        DawnProcTable table;
-        table.getProcAddress = ClientGetProcAddress;
-        table.createInstance = ClientCreateInstance;
+    static DawnProcTable gProcTable = {
+        ClientGetProcAddress,
+        ClientCreateInstance,
         {% for type in by_category["object"] %}
             {% for method in c_methods(type) %}
-                {% set suffix = as_MethodSuffix(type.name, method.name) %}
-                table.{{as_varName(type.name, method.name)}} = Client{{suffix}};
+                Client{{as_MethodSuffix(type.name, method.name)}},
             {% endfor %}
         {% endfor %}
-        return table;
+    };
+    const DawnProcTable& GetProcs() {
+        return gProcTable;
     }
 }}  // namespace dawn_wire::client

src/dawn/BUILD.gn

@@ -87,7 +87,10 @@ source_set("dawncpp") {
 
 dawn_json_generator("dawn_proc_gen") {
   target = "dawn_proc"
-  outputs = [ "src/dawn/dawn_proc.c" ]
+  outputs = [
+    "src/dawn/dawn_proc.c",
+    "src/dawn/dawn_thread_dispatch_proc.cpp",
+  ]
 }
 
 dawn_component("dawn_proc") {
@@ -96,5 +99,8 @@ dawn_component("dawn_proc") {
   public_deps = [ ":dawn_headers" ]
   deps = [ ":dawn_proc_gen" ]
   sources = get_target_outputs(":dawn_proc_gen")
-  sources += [ "${dawn_root}/src/include/dawn/dawn_proc.h" ]
+  sources += [
+    "${dawn_root}/src/include/dawn/dawn_proc.h",
+    "${dawn_root}/src/include/dawn/dawn_thread_dispatch_proc.h",
+  ]
 }

src/dawn_native/DawnNative.cpp

@@ -22,9 +22,9 @@
 
 namespace dawn_native {
 
-    DawnProcTable GetProcsAutogen();
+    const DawnProcTable& GetProcsAutogen();
 
-    DawnProcTable GetProcs() {
+    const DawnProcTable& GetProcs() {
         return GetProcsAutogen();
     }
 

src/dawn_wire/WireClient.cpp

@@ -26,7 +26,7 @@ namespace dawn_wire {
     }
 
     // static
-    DawnProcTable WireClient::GetProcs() {
+    const DawnProcTable& WireClient::GetProcs() {
         return client::GetProcs();
     }
 

src/dawn_wire/client/Client.h

@@ -68,8 +68,6 @@ namespace dawn_wire { namespace client {
         bool mIsDisconnected = false;
     };
 
-    DawnProcTable GetProcs();
-
     std::unique_ptr<MemoryTransferService> CreateInlineMemoryTransferService();
 
 }}  // namespace dawn_wire::client

src/include/dawn/dawn_thread_dispatch_proc.h

@@ -0,0 +1,33 @@
+// Copyright 2020 The Dawn Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef DAWN_DAWN_THREAD_DISPATCH_PROC_H_
+#define DAWN_DAWN_THREAD_DISPATCH_PROC_H_
+
+#include "dawn/dawn_proc.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+// Call dawnProcSetProcs(&dawnThreadDispatchProcTable) and then use dawnProcSetPerThreadProcs
+// to set per-thread procs.
+WGPU_EXPORT extern DawnProcTable dawnThreadDispatchProcTable;
+WGPU_EXPORT void dawnProcSetPerThreadProcs(const DawnProcTable* procs);
+
+#ifdef __cplusplus
+}  // extern "C"
+#endif
+
+#endif  // DAWN_DAWN_THREAD_DISPATCH_PROC_H_

src/include/dawn_native/DawnNative.h

@@ -170,7 +170,7 @@ namespace dawn_native {
     };
 
     // Backend-agnostic API for dawn_native
-    DAWN_NATIVE_EXPORT DawnProcTable GetProcs();
+    DAWN_NATIVE_EXPORT const DawnProcTable& GetProcs();
 
     // Query the names of all the toggles that are enabled in device
     DAWN_NATIVE_EXPORT std::vector<const char*> GetTogglesUsed(WGPUDevice device);

src/include/dawn_wire/WireClient.h

@@ -26,6 +26,8 @@ namespace dawn_wire {
     namespace client {
         class Client;
         class MemoryTransferService;
+
+        DAWN_WIRE_EXPORT const DawnProcTable& GetProcs();
     }  // namespace client
 
     struct ReservedTexture {
@@ -44,7 +46,8 @@ namespace dawn_wire {
         WireClient(const WireClientDescriptor& descriptor);
         ~WireClient() override;
 
-        static DawnProcTable GetProcs();
+        // TODO(enga): Remove this and use dawn_wire::client::GetProcs() instead
+        static const DawnProcTable& GetProcs();
 
         WGPUDevice GetDevice() const;
         const volatile char* HandleCommands(const volatile char* commands,

src/tests/BUILD.gn

@@ -168,6 +168,7 @@ test("dawn_unittests") {
     "unittests/MathTests.cpp",
     "unittests/ObjectBaseTests.cpp",
     "unittests/PerStageTests.cpp",
+    "unittests/PerThreadProcTests.cpp",
     "unittests/PlacementAllocatedTests.cpp",
     "unittests/RefCountedTests.cpp",
     "unittests/ResultTests.cpp",

src/tests/DawnTest.cpp

@@ -744,12 +744,9 @@ void DawnTestBase::SetUp() {
         clientDesc.serializer = mC2sBuf.get();
 
         mWireClient.reset(new dawn_wire::WireClient(clientDesc));
-        WGPUDevice clientDevice = mWireClient->GetDevice();
-        DawnProcTable clientProcs = dawn_wire::WireClient::GetProcs();
+        cDevice = mWireClient->GetDevice();
+        procs = dawn_wire::client::GetProcs();
         mS2cBuf->SetHandler(mWireClient.get());
-
-        procs = clientProcs;
-        cDevice = clientDevice;
     } else {
         procs = backendProcs;
         cDevice = backendDevice;

src/tests/end2end/WindowSurfaceTests.cpp

@@ -50,8 +50,7 @@ class WindowSurfaceInstanceTests : public testing::Test {
         });
         DAWN_SKIP_TEST_IF(!glfwInit());
 
-        DawnProcTable procs = dawn_native::GetProcs();
-        dawnProcSetProcs(&procs);
+        dawnProcSetProcs(&dawn_native::GetProcs());
 
         mInstance = wgpu::CreateInstance();
     }

src/tests/unittests/GetProcAddressTests.cpp

@@ -72,7 +72,7 @@ namespace {
                     mWireClient = std::make_unique<dawn_wire::WireClient>(clientDesc);
 
                     mDevice = wgpu::Device::Acquire(mWireClient->GetDevice());
-                    mProcs = dawn_wire::WireClient::GetProcs();
+                    mProcs = dawn_wire::client::GetProcs();
                     break;
                 }
 

src/tests/unittests/PerThreadProcTests.cpp

@@ -0,0 +1,118 @@
+// Copyright 2020 The Dawn Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "dawn/dawn_thread_dispatch_proc.h"
+#include "dawn/webgpu_cpp.h"
+#include "dawn_native/DawnNative.h"
+#include "dawn_native/Instance.h"
+#include "dawn_native/null/DeviceNull.h"
+
+#include <gtest/gtest.h>
+#include <atomic>
+#include <thread>
+
+class PerThreadProcTests : public testing::Test {
+  public:
+    PerThreadProcTests()
+        : mNativeInstance(dawn_native::InstanceBase::Create()),
+          mNativeAdapter(mNativeInstance.Get()) {
+    }
+    ~PerThreadProcTests() override = default;
+
+  protected:
+    Ref<dawn_native::InstanceBase> mNativeInstance;
+    dawn_native::null::Adapter mNativeAdapter;
+};
+
+// Test that procs can be set per thread. This test overrides deviceCreateBuffer with a dummy proc
+// for each thread that increments a counter. Because each thread has their own proc and counter,
+// there should be no data races. The per-thread procs also check that the current thread id is
+// exactly equal to the expected thread id.
+TEST_F(PerThreadProcTests, DispatchesPerThread) {
+    dawnProcSetProcs(&dawnThreadDispatchProcTable);
+
+    // Threads will block on this atomic to be sure we set procs on both threads before
+    // either thread calls the procs.
+    std::atomic<bool> ready(false);
+
+    static int threadACounter = 0;
+    static int threadBCounter = 0;
+
+    static std::atomic<std::thread::id> threadIdA;
+    static std::atomic<std::thread::id> threadIdB;
+
+    constexpr int kThreadATargetCount = 28347;
+    constexpr int kThreadBTargetCount = 40420;
+
+    // Note: Acquire doesn't call reference or release.
+    wgpu::Device deviceA =
+        wgpu::Device::Acquire(reinterpret_cast<WGPUDevice>(mNativeAdapter.CreateDevice(nullptr)));
+
+    wgpu::Device deviceB =
+        wgpu::Device::Acquire(reinterpret_cast<WGPUDevice>(mNativeAdapter.CreateDevice(nullptr)));
+
+    std::thread threadA([&]() {
+        DawnProcTable procs = dawn_native::GetProcs();
+        procs.deviceCreateBuffer = [](WGPUDevice device,
+                                      WGPUBufferDescriptor const* descriptor) -> WGPUBuffer {
+            EXPECT_EQ(std::this_thread::get_id(), threadIdA);
+            threadACounter++;
+            return nullptr;
+        };
+        dawnProcSetPerThreadProcs(&procs);
+
+        while (!ready) {
+        }  // Should be fast, so just spin.
+
+        for (int i = 0; i < kThreadATargetCount; ++i) {
+            deviceA.CreateBuffer(nullptr);
+        }
+
+        deviceA = nullptr;
+        dawnProcSetPerThreadProcs(nullptr);
+    });
+
+    std::thread threadB([&]() {
+        DawnProcTable procs = dawn_native::GetProcs();
+        procs.deviceCreateBuffer = [](WGPUDevice device,
+                                      WGPUBufferDescriptor const* bufferDesc) -> WGPUBuffer {
+            EXPECT_EQ(std::this_thread::get_id(), threadIdB);
+            threadBCounter++;
+            return nullptr;
+        };
+        dawnProcSetPerThreadProcs(&procs);
+
+        while (!ready) {
+        }  // Should be fast, so just spin.
+
+        for (int i = 0; i < kThreadBTargetCount; ++i) {
+            deviceB.CreateBuffer(nullptr);
+        }
+
+        deviceB = nullptr;
+        dawnProcSetPerThreadProcs(nullptr);
+    });
+
+    threadIdA = threadA.get_id();
+    threadIdB = threadB.get_id();
+
+    ready = true;
+    threadA.join();
+    threadB.join();
+
+    EXPECT_EQ(threadACounter, kThreadATargetCount);
+    EXPECT_EQ(threadBCounter, kThreadBTargetCount);
+
+    dawnProcSetProcs(nullptr);
+}

src/tests/unittests/validation/ValidationTest.cpp

@@ -40,8 +40,7 @@ ValidationTest::ValidationTest() {
 
     ASSERT(foundNullAdapter);
 
-    DawnProcTable procs = dawn_native::GetProcs();
-    dawnProcSetProcs(&procs);
+    dawnProcSetProcs(&dawn_native::GetProcs());
 
     device = CreateDeviceFromAdapter(adapter, std::vector<const char*>());
 }

src/tests/unittests/wire/WireMultipleDeviceTests.cpp

@@ -29,8 +29,7 @@ using namespace dawn_wire;
 class WireMultipleDeviceTests : public testing::Test {
   protected:
     void SetUp() override {
-        DawnProcTable procs = dawn_wire::WireClient::GetProcs();
-        dawnProcSetProcs(&procs);
+        dawnProcSetProcs(&dawn_wire::client::GetProcs());
     }
 
     void TearDown() override {

src/tests/unittests/wire/WireTest.cpp

@@ -66,8 +66,7 @@ void WireTest::SetUp() {
     mS2cBuf->SetHandler(mWireClient.get());
 
     device = mWireClient->GetDevice();
-    DawnProcTable clientProcs = dawn_wire::WireClient::GetProcs();
-    dawnProcSetProcs(&clientProcs);
+    dawnProcSetProcs(&dawn_wire::client::GetProcs());
 
     apiDevice = mockDevice;