SpirvShaderDebugger: Reduce lock contention

src/Pipeline/SpirvShaderDebugger.cpp

@@ -261,7 +261,7 @@ struct Type : public Object
 
 	// 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 @@ struct BasicType : ObjectImpl<BasicType, Type, Object::Kind::BasicType>
 
 	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 @@ struct ArrayType : ObjectImpl<ArrayType, Type, Object::Kind::ArrayType>
 		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 @@ struct VectorType : ObjectImpl<VectorType, Type, Object::Kind::VectorType>
 		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 @@ struct VectorType : ObjectImpl<VectorType, Type, Object::Kind::VectorType>
 #	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 @@ struct FunctionType : ObjectImpl<FunctionType, Type, Object::Kind::FunctionType>
 	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 @@ struct CompositeType : ObjectImpl<CompositeType, Type, Object::Kind::CompositeTy
 	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 @@ struct CompositeType : ObjectImpl<CompositeType, Type, Object::Kind::CompositeTy
 #	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 @@ struct TemplateType : ObjectImpl<TemplateType, Type, Object::Kind::TemplateType>
 	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 @@ SpirvShader::Impl::Debugger::State::State(const Debugger *debugger, const char *
 		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 @@ vk::dbg::VariableContainer *SpirvShader::Impl::Debugger::State::localsLane(const
 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 @@ void SpirvShader::Impl::Debugger::State::putVal(vk::dbg::VariableContainer *vc,
 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 @@ void SpirvShader::Impl::Debugger::State::createScope(const debug::Scope *spirvSc
 
 	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);
 	}

src/Vulkan/CMakeLists.txt

@@ -119,6 +119,7 @@ if(SWIFTSHADER_ENABLE_VULKAN_DEBUGGER)
         Debug/Type.hpp
         Debug/Value.cpp
         Debug/Value.hpp
+        Debug/Variable.cpp
         Debug/Variable.hpp
         Debug/WeakMap.hpp
     )

src/Vulkan/Debug/Context.cpp

@@ -311,7 +311,7 @@ std::shared_ptr<Frame> Context::Lock::get(Frame::ID id)
 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 @@ std::shared_ptr<Scope> Context::Lock::get(Scope::ID id)
 	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 @@ bool Context::Lock::isFunctionBreakpoint(const std::string &name)
 }
 
 }  // namespace dbg
-}  // namespace vk
+}  // namespace vk
\ No newline at end of file

src/Vulkan/Debug/Context.hpp

@@ -106,8 +106,11 @@ public:
 		// 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

src/Vulkan/Debug/Server.cpp

@@ -55,7 +55,7 @@ public:
 	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 @@ Server::Impl::Impl(const std::shared_ptr<Context> &context, int port)
 
 		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 @@ Server::Impl::Impl(const std::shared_ptr<Context> &context, int port)
 			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 @@ void Server::Impl::onFunctionBreakpointHit(ID<Thread> id)
 	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 @@ dap::Scope Server::Impl::scope(const char *type, Scope *s)
 	out.name = type;
 	out.presentationHint = type;
 	out.variablesReference = s->variables->id.value();
+	lock.track(s->variables);
 	return out;
 }
 
@@ -588,4 +590,4 @@ std::shared_ptr<Server> Server::create(const std::shared_ptr<Context> &ctx, int
 }
 
 }  // namespace dbg
-}  // namespace vk
+}  // namespace vk
\ No newline at end of file

src/Vulkan/Debug/Variable.cpp

+// 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

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 @@ class VariableContainer : public Value
 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 @@ public:
 	const ID id;
 
 private:
+	static std::atomic<int> nextID;
+
 	struct ForeachIndex
 	{
 		size_t start;
@@ -89,8 +92,8 @@ private:
 	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 @@ const void *VariableContainer::get() const
 }  // namespace dbg
 }  // namespace vk
 
-#endif  // VK_DEBUG_VARIABLE_HPP_
+#endif  // VK_DEBUG_VARIABLE_HPP_
\ No newline at end of file