SpirvShaderDebugger: Reduce lock contention
Previously the way to create `VariableContainer`s was to call
`createVariableContainer()` on the `Context::Lock`. This was required so that debug-client requests to inspect the container's contents can be done with a simple map lookup.
Creating variable containers is a high frequency operation, and obtaining a `Context::Lock` (unsurprisingly) requires acquiring a mutex lock, which across multiple threads results in significant mutex lock contention.
To dramatically help with debugger performance, we can now construct `VariableContainer`s outside of the lock. This is achieved by registering the variable container in the context map only when it is handed to the client. This only occurs when there's a DAP transaction, which is orders of magnitude less frequent.
Bug: b/148401179
Change-Id: I58eca33aac8fd4711dd9756072a6a06efdacc671
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/48428
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Jaebaek Seo <jaebaek@google.com>
Reviewed-by: Nicolas Capens <nicolascapens@google.com>
Tested-by: Ben Clayton <bclayton@google.com>
diff --git a/src/Pipeline/SpirvShaderDebugger.cpp b/src/Pipeline/SpirvShaderDebugger.cpp
index 28993f7..06f75c9 100644
--- a/src/Pipeline/SpirvShaderDebugger.cpp
+++ b/src/Pipeline/SpirvShaderDebugger.cpp
@@ -261,7 +261,7 @@
// value() returns a shared pointer to a vk::dbg::Value that views the data
// at ptr of this type.
- virtual std::shared_ptr<vk::dbg::Value> value(vk::dbg::Context::Lock &lock, void *ptr, bool interleaved) const = 0;
+ virtual std::shared_ptr<vk::dbg::Value> value(void *ptr, bool interleaved) const = 0;
};
struct CompilationUnit : ObjectImpl<CompilationUnit, Scope, Object::Kind::CompilationUnit>
@@ -286,7 +286,7 @@
uint32_t sizeInBytes() const override { return size / 8; }
- std::shared_ptr<vk::dbg::Value> value(vk::dbg::Context::Lock &lock, void *ptr, bool interleaved) const override
+ std::shared_ptr<vk::dbg::Value> value(void *ptr, bool interleaved) const override
{
switch(encoding)
{
@@ -397,20 +397,20 @@
return numBytes;
}
- std::shared_ptr<vk::dbg::Value> value(vk::dbg::Context::Lock &lock, void *ptr, bool interleaved) const override
+ std::shared_ptr<vk::dbg::Value> value(void *ptr, bool interleaved) const override
{
- auto vc = lock.createVariableContainer();
+ auto vc = std::make_shared<vk::dbg::VariableContainer>();
build(
vc,
[&](std::shared_ptr<vk::dbg::VariableContainer> &parent, uint32_t idx) {
- auto child = lock.createVariableContainer();
+ auto child = std::make_shared<vk::dbg::VariableContainer>();
parent->put(tostring(idx), child);
return child;
},
[&](std::shared_ptr<vk::dbg::VariableContainer> &parent, uint32_t idx, uint32_t offset) {
offset = offset * base->sizeInBytes() * (interleaved ? sw::SIMD::Width : 1);
auto addr = static_cast<uint8_t *>(ptr) + offset;
- auto child = base->value(lock, addr, interleaved);
+ auto child = base->value(addr, interleaved);
auto key = tostring(idx);
# if DEBUG_ANNOTATE_VARIABLE_KEYS
key += " (" + tostring(addr) + " +" + tostring(offset) + ", idx: " + tostring(idx) + ")" + (interleaved ? "I" : "F");
@@ -431,10 +431,10 @@
return base->sizeInBytes() * components;
}
- std::shared_ptr<vk::dbg::Value> value(vk::dbg::Context::Lock &lock, void *ptr, bool interleaved) const override
+ std::shared_ptr<vk::dbg::Value> value(void *ptr, bool interleaved) const override
{
const auto elSize = base->sizeInBytes();
- auto vc = lock.createVariableContainer();
+ auto vc = std::make_shared<vk::dbg::VariableContainer>();
for(uint32_t i = 0; i < components; i++)
{
auto offset = elSize * i * (interleaved ? sw::SIMD::Width : 1);
@@ -443,7 +443,7 @@
# if DEBUG_ANNOTATE_VARIABLE_KEYS
elKey += " (" + tostring(elPtr) + " +" + tostring(offset) + ")" + (interleaved ? "I" : "F");
# endif
- vc->put(elKey, base->value(lock, elPtr, interleaved));
+ vc->put(elKey, base->value(elPtr, interleaved));
}
return vc;
}
@@ -456,7 +456,7 @@
std::vector<Type *> paramTys;
uint32_t sizeInBytes() const override { return 0; }
- std::shared_ptr<vk::dbg::Value> value(vk::dbg::Context::Lock &lock, void *ptr, bool interleaved) const override { return nullptr; }
+ std::shared_ptr<vk::dbg::Value> value(void *ptr, bool interleaved) const override { return nullptr; }
};
struct Member : ObjectImpl<Member, Object, Object::Kind::Member>
@@ -486,9 +486,10 @@
std::vector<Member *> members;
uint32_t sizeInBytes() const override { return size / 8; }
- std::shared_ptr<vk::dbg::Value> value(vk::dbg::Context::Lock &lock, void *ptr, bool interleaved) const override
+
+ std::shared_ptr<vk::dbg::Value> value(void *ptr, bool interleaved) const override
{
- auto vc = lock.createVariableContainer();
+ auto vc = std::make_shared<vk::dbg::VariableContainer>();
for(auto &member : members)
{
auto offset = (member->offset / 8) * (interleaved ? sw::SIMD::Width : 1);
@@ -497,7 +498,7 @@
# if DEBUG_ANNOTATE_VARIABLE_KEYS
// elKey += " (" + tostring(elPtr) + " +" + tostring(offset) + ")" + (interleaved ? "I" : "F");
# endif
- vc->put(elKey, member->type->value(lock, elPtr, interleaved));
+ vc->put(elKey, member->type->value(elPtr, interleaved));
}
return vc;
}
@@ -519,9 +520,9 @@
std::vector<TemplateParameter *> parameters;
uint32_t sizeInBytes() const override { return target->sizeInBytes(); }
- std::shared_ptr<vk::dbg::Value> value(vk::dbg::Context::Lock &lock, void *ptr, bool interleaved) const override
+ std::shared_ptr<vk::dbg::Value> value(void *ptr, bool interleaved) const override
{
- return target->value(lock, ptr, interleaved);
+ return target->value(ptr, interleaved);
}
};
@@ -829,7 +830,7 @@
globals.hovers = frame.hovers;
for(int i = 0; i < sw::SIMD::Width; i++)
{
- auto locals = lock.createVariableContainer();
+ auto locals = std::make_shared<vk::dbg::VariableContainer>();
frame.locals->variables->put(laneNames[i], locals);
globals.localsByLane[i] = locals;
}
@@ -880,7 +881,7 @@
template<typename K>
vk::dbg::VariableContainer *SpirvShader::Impl::Debugger::State::group(vk::dbg::VariableContainer *vc, K key)
{
- auto out = debugger->ctx->lock().createVariableContainer();
+ auto out = std::make_shared<vk::dbg::VariableContainer>();
vc->put(tostring(key), out);
return out.get();
}
@@ -894,8 +895,7 @@
template<typename K>
void SpirvShader::Impl::Debugger::State::putPtr(vk::dbg::VariableContainer *vc, K key, void *ptr, bool interleaved, const debug::Type *type)
{
- auto lock = debugger->ctx->lock();
- vc->put(tostring(key), type->value(lock, ptr, interleaved));
+ vc->put(tostring(key), type->value(ptr, interleaved));
}
template<typename K, typename V>
@@ -919,7 +919,7 @@
for(int i = 0; i < sw::SIMD::Width; i++)
{
- auto locals = lock.createVariableContainer();
+ auto locals = std::make_shared<vk::dbg::VariableContainer>();
s.localsByLane[i] = locals;
s.locals->variables->put(laneNames[i], locals);
}
diff --git a/src/Vulkan/CMakeLists.txt b/src/Vulkan/CMakeLists.txt
index bbd53cd..58ca289 100644
--- a/src/Vulkan/CMakeLists.txt
+++ b/src/Vulkan/CMakeLists.txt
@@ -119,6 +119,7 @@
Debug/Type.hpp
Debug/Value.cpp
Debug/Value.hpp
+ Debug/Variable.cpp
Debug/Variable.hpp
Debug/WeakMap.hpp
)
diff --git a/src/Vulkan/Debug/Context.cpp b/src/Vulkan/Debug/Context.cpp
index f79d345..eb43b4a 100644
--- a/src/Vulkan/Debug/Context.cpp
+++ b/src/Vulkan/Debug/Context.cpp
@@ -311,7 +311,7 @@
std::shared_ptr<Scope> Context::Lock::createScope(
const std::shared_ptr<File> &file)
{
- auto scope = std::make_shared<Scope>(ctx->nextScopeID++, file, createVariableContainer());
+ auto scope = std::make_shared<Scope>(ctx->nextScopeID++, file, std::make_shared<VariableContainer>());
ctx->scopes.add(scope->id, scope);
return scope;
}
@@ -321,11 +321,9 @@
return ctx->scopes.get(id);
}
-std::shared_ptr<VariableContainer> Context::Lock::createVariableContainer()
+void Context::Lock::track(const std::shared_ptr<VariableContainer> &vc)
{
- auto container = std::make_shared<VariableContainer>(ctx->nextVariableContainerID++);
- ctx->variableContainers.add(container->id, container);
- return container;
+ ctx->variableContainers.add(vc->id, vc);
}
std::shared_ptr<VariableContainer> Context::Lock::get(VariableContainer::ID id)
@@ -349,4 +347,4 @@
}
} // namespace dbg
-} // namespace vk
\ No newline at end of file
+} // namespace vk
diff --git a/src/Vulkan/Debug/Context.hpp b/src/Vulkan/Debug/Context.hpp
index 4c4d8af..92f3086 100644
--- a/src/Vulkan/Debug/Context.hpp
+++ b/src/Vulkan/Debug/Context.hpp
@@ -106,8 +106,11 @@
// does not exist.
std::shared_ptr<Scope> get(ID<Scope>);
- // createVariableContainer() returns a new variable container.
- std::shared_ptr<VariableContainer> createVariableContainer();
+ // track() registers the variable container with the context so it can
+ // be retrieved by get(). Note that the context does not hold a strong
+ // reference to the variable container, and get() will return nullptr
+ // if all strong external references are dropped.
+ void track(const std::shared_ptr<VariableContainer> &);
// get() returns the variable container with the given ID, or null if
// the variable container does not exist or no longer has any external
diff --git a/src/Vulkan/Debug/Server.cpp b/src/Vulkan/Debug/Server.cpp
index 667caee..6b3b692 100644
--- a/src/Vulkan/Debug/Server.cpp
+++ b/src/Vulkan/Debug/Server.cpp
@@ -55,7 +55,7 @@
void onLineBreakpointHit(ID<Thread>) override;
void onFunctionBreakpointHit(ID<Thread>) override;
- dap::Scope scope(const char *type, Scope *);
+ dap::Scope scope(Context::Lock &lock, const char *type, Scope *);
dap::Source source(File *);
std::shared_ptr<File> file(const dap::Source &source);
@@ -230,9 +230,9 @@
dap::ScopesResponse response;
response.scopes = {
- scope("locals", frame->locals.get()),
- scope("arguments", frame->arguments.get()),
- scope("registers", frame->registers.get()),
+ scope(lock, "locals", frame->locals.get()),
+ scope(lock, "arguments", frame->arguments.get()),
+ scope(lock, "registers", frame->registers.get()),
};
return response;
});
@@ -258,8 +258,9 @@
out.value = v.value->string();
if(v.value->type()->kind == Kind::VariableContainer)
{
- auto const vc = static_cast<const VariableContainer *>(v.value.get());
+ auto const vc = std::dynamic_pointer_cast<VariableContainer>(v.value);
out.variablesReference = vc->id.value();
+ lock.track(vc);
}
response.variables.push_back(out);
});
@@ -512,7 +513,7 @@
session->send(event);
}
-dap::Scope Server::Impl::scope(const char *type, Scope *s)
+dap::Scope Server::Impl::scope(Context::Lock &lock, const char *type, Scope *s)
{
dap::Scope out;
// out.line = s->startLine;
@@ -521,6 +522,7 @@
out.name = type;
out.presentationHint = type;
out.variablesReference = s->variables->id.value();
+ lock.track(s->variables);
return out;
}
@@ -588,4 +590,4 @@
}
} // namespace dbg
-} // namespace vk
\ No newline at end of file
+} // namespace vk
diff --git a/src/Vulkan/Debug/Variable.cpp b/src/Vulkan/Debug/Variable.cpp
new file mode 100644
index 0000000..8145e01
--- /dev/null
+++ b/src/Vulkan/Debug/Variable.cpp
@@ -0,0 +1,23 @@
+// Copyright 2019 The SwiftShader Authors. All Rights Reserved.
+//
+// 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 "Variable.hpp"
+
+namespace vk {
+namespace dbg {
+
+std::atomic<int> VariableContainer::nextID{};
+
+} // namespace dbg
+} // namespace vk
diff --git a/src/Vulkan/Debug/Variable.hpp b/src/Vulkan/Debug/Variable.hpp
index c916d75..8818b5b 100644
--- a/src/Vulkan/Debug/Variable.hpp
+++ b/src/Vulkan/Debug/Variable.hpp
@@ -23,6 +23,7 @@
#include "marl/tsa.h"
#include <algorithm>
+#include <atomic>
#include <memory>
#include <string>
#include <unordered_map>
@@ -44,7 +45,7 @@
public:
using ID = dbg::ID<VariableContainer>;
- inline VariableContainer(ID id);
+ inline VariableContainer();
// foreach() calls cb with each of the variables in the container.
// F must be a function with the signature void(const Variable&).
@@ -71,6 +72,8 @@
const ID id;
private:
+ static std::atomic<int> nextID;
+
struct ForeachIndex
{
size_t start;
@@ -89,8 +92,8 @@
std::vector<std::shared_ptr<VariableContainer>> extends GUARDED_BY(mutex);
};
-VariableContainer::VariableContainer(ID id)
- : id(id)
+VariableContainer::VariableContainer()
+ : id(nextID++)
{}
template<typename F>
@@ -180,4 +183,4 @@
} // namespace dbg
} // namespace vk
-#endif // VK_DEBUG_VARIABLE_HPP_
\ No newline at end of file
+#endif // VK_DEBUG_VARIABLE_HPP_
