Merge pull request #165 from billhollings/master

Synchronization and threading fixes.

MoltenVK/MoltenVK/API/vk_mvk_moltenvk.h

@@ -48,7 +48,7 @@ extern "C" {
  */
 #define MVK_VERSION_MAJOR 1
 #define MVK_VERSION_MINOR 0
-#define MVK_VERSION_PATCH 8
+#define MVK_VERSION_PATCH 9
 
 #define MVK_MAKE_VERSION(major, minor, patch) (((major) * 10000) + ((minor) * 100) + (patch))
 #define MVK_VERSION MVK_MAKE_VERSION(MVK_VERSION_MAJOR, MVK_VERSION_MINOR, MVK_VERSION_PATCH)

MoltenVK/MoltenVK/GPUObjects/MVKPipeline.h

@@ -228,7 +228,7 @@ class MVKRenderPipelineCompiler : public MVKMetalCompiler {
  ~MVKRenderPipelineCompiler() override;
 
 protected:
- void compileComplete(id<MTLRenderPipelineState> pipelineState, NSError *error);
+ bool compileComplete(id<MTLRenderPipelineState> pipelineState, NSError *error);
 
  id<MTLRenderPipelineState> _mtlRenderPipelineState = nil;
 };
@@ -265,7 +265,7 @@ class MVKComputePipelineCompiler : public MVKMetalCompiler {
  ~MVKComputePipelineCompiler() override;
 
 protected:
- void compileComplete(id<MTLComputePipelineState> pipelineState, NSError *error);
+ bool compileComplete(id<MTLComputePipelineState> pipelineState, NSError *error);
 
  id<MTLComputePipelineState> _mtlComputePipelineState = nil;
 };

MoltenVK/MoltenVK/GPUObjects/MVKPipeline.mm

@@ -747,18 +747,19 @@ void serialize(Archive & archive, MVKShaderModuleKey& k) {
  compile(lock, ^{
  [getMTLDevice() newRenderPipelineStateWithDescriptor: mtlRPLDesc
  completionHandler: ^(id<MTLRenderPipelineState> ps, NSError* error) {
- compileComplete(ps, error);
+ bool isLate = compileComplete(ps, error);
+ if (isLate) { destroy(); }
  }];
  });
 
  return [_mtlRenderPipelineState retain];
 }
 
-void MVKRenderPipelineCompiler::compileComplete(id<MTLRenderPipelineState> mtlRenderPipelineState, NSError* compileError) {
+bool MVKRenderPipelineCompiler::compileComplete(id<MTLRenderPipelineState> mtlRenderPipelineState, NSError* compileError) {
  lock_guard<mutex> lock(_completionLock);
 
  _mtlRenderPipelineState = [mtlRenderPipelineState retain]; // retained
- endCompile(compileError);
+ return endCompile(compileError);
 }
 
 #pragma mark Construction
@@ -777,18 +778,19 @@ void serialize(Archive & archive, MVKShaderModuleKey& k) {
  compile(lock, ^{
  [getMTLDevice() newComputePipelineStateWithFunction: mtlFunction
  completionHandler: ^(id<MTLComputePipelineState> ps, NSError* error) {
- compileComplete(ps, error);
+ bool isLate = compileComplete(ps, error);
+ if (isLate) { destroy(); }
  }];
  });
 
  return [_mtlComputePipelineState retain];
 }
 
-void MVKComputePipelineCompiler::compileComplete(id<MTLComputePipelineState> mtlComputePipelineState, NSError* compileError) {
+bool MVKComputePipelineCompiler::compileComplete(id<MTLComputePipelineState> mtlComputePipelineState, NSError* compileError) {
  lock_guard<mutex> lock(_completionLock);
 
  _mtlComputePipelineState = [mtlComputePipelineState retain]; // retained
- endCompile(compileError);
+ return endCompile(compileError);
 }
 
 #pragma mark Construction

MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.h

@@ -212,7 +212,7 @@ class MVKShaderLibraryCompiler : public MVKMetalCompiler {
  ~MVKShaderLibraryCompiler() override;
 
 protected:
- void compileComplete(id<MTLLibrary> mtlLibrary, NSError *error);
+ bool compileComplete(id<MTLLibrary> mtlLibrary, NSError *error);
  void handleError() override;
 
  id<MTLLibrary> _mtlLibrary = nil;
@@ -250,7 +250,7 @@ class MVKFunctionSpecializer : public MVKMetalCompiler {
  ~MVKFunctionSpecializer() override;
 
 protected:
- void compileComplete(id<MTLFunction> mtlFunction, NSError *error);
+ bool compileComplete(id<MTLFunction> mtlFunction, NSError *error);
 
  id<MTLFunction> _mtlFunction = nil;
 };

MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.mm

@@ -350,7 +350,8 @@ static uint32_t getOffsetForConstantId(const VkSpecializationInfo* pSpecInfo, ui
  [getMTLDevice() newLibraryWithSource: mslSourceCode
  options: options
  completionHandler: ^(id<MTLLibrary> mtlLib, NSError* error) {
- compileComplete(mtlLib, error);
+ bool isLate = compileComplete(mtlLib, error);
+ if (isLate) { destroy(); }
  }];
  });
 
@@ -366,11 +367,11 @@ static uint32_t getOffsetForConstantId(const VkSpecializationInfo* pSpecInfo, ui
  }
 }
 
-void MVKShaderLibraryCompiler::compileComplete(id<MTLLibrary> mtlLibrary, NSError* compileError) {
+bool MVKShaderLibraryCompiler::compileComplete(id<MTLLibrary> mtlLibrary, NSError* compileError) {
  lock_guard<mutex> lock(_completionLock);
 
  _mtlLibrary = [mtlLibrary retain]; // retained
- endCompile(compileError);
+ return endCompile(compileError);
 }
 
 #pragma mark Construction
@@ -392,18 +393,19 @@ static uint32_t getOffsetForConstantId(const VkSpecializationInfo* pSpecInfo, ui
  [mtlLibrary newFunctionWithName: funcName
  constantValues: constantValues
  completionHandler: ^(id<MTLFunction> mtlFunc, NSError* error) {
- compileComplete(mtlFunc, error);
+ bool isLate = compileComplete(mtlFunc, error);
+ if (isLate) { destroy(); }
  }];
  });
 
  return [_mtlFunction retain];
 }
 
-void MVKFunctionSpecializer::compileComplete(id<MTLFunction> mtlFunction, NSError* compileError) {
+bool MVKFunctionSpecializer::compileComplete(id<MTLFunction> mtlFunction, NSError* compileError) {
  lock_guard<mutex> lock(_completionLock);
 
  _mtlFunction = [mtlFunction retain]; // retained
- endCompile(compileError);
+ return endCompile(compileError);
 }
 
 #pragma mark Construction

MoltenVK/MoltenVK/GPUObjects/MVKSurface.mm

@@ -19,6 +19,7 @@
 #include "MVKSurface.h"
 #include "MVKInstance.h"
 #include "MVKFoundation.h"
+#include "MVKOSExtensions.h"
 
 
 #pragma mark MVKSurface
@@ -29,7 +30,8 @@
  const Vk_PLATFORM_SurfaceCreateInfoMVK* pCreateInfo,
  const VkAllocationCallbacks* pAllocator) {
 
- CALayer* viewLayer = ((PLATFORM_VIEW_CLASS*)pCreateInfo->pView).layer;
+ __block CALayer* viewLayer = nil;
+ mvkDispatchToMainAndWait(^{ viewLayer = ((PLATFORM_VIEW_CLASS*)pCreateInfo->pView).layer; });
  if ( [viewLayer isKindOfClass: [CAMetalLayer class]] ) {
  _mtlCAMetalLayer = (CAMetalLayer*)[viewLayer retain]; // retained
  } else {

MoltenVK/MoltenVK/GPUObjects/MVKSync.h

@@ -22,6 +22,7 @@
 #include <mutex>
 #include <condition_variable>
 #include <unordered_set>
+#include <vector>
 
 class MVKFenceSitter;
 
@@ -122,7 +123,8 @@ class MVKSignalable : public MVKBaseDeviceObject {
  MVKSignalable(MVKDevice* device) : MVKBaseDeviceObject(device) {}
 
 protected:
- void maybeDestroy();
+ bool decrementSignalerCount();
+ bool markDestroyed();
 
  std::mutex _signalerLock;
  uint32_t _signalerCount = 0;
@@ -246,6 +248,7 @@ class MVKFenceSitter : public MVKBaseObject {
 
  void addUnsignaledFence(MVKFence* fence);
  void fenceSignaled(MVKFence* fence);
+ void getUnsignaledFences(std::vector<MVKFence*>& fences);
 
  std::mutex _lock;
  std::unordered_set<MVKFence*> _unsignaledFences;
@@ -295,8 +298,8 @@ class MVKMetalCompiler : public MVKBaseDeviceObject {
 protected:
  void compile(std::unique_lock<std::mutex>& lock, dispatch_block_t block);
  virtual void handleError();
- void endCompile(NSError* compileError);
- void maybeDestroy();
+ bool endCompile(NSError* compileError);
+ bool markDestroyed();
 
  NSError* _compileError = nil;
  uint64_t _startTime = 0;

MoltenVK/MoltenVK/GPUObjects/MVKSync.mm

@@ -75,23 +75,27 @@
 }
 
 void MVKSignalable::wasRemovedFromSignaler() {
+ if (decrementSignalerCount()) { destroy(); }
+}
+
+// Decrements the signaler count, and returns whether it's time to destroy this object.
+bool MVKSignalable::decrementSignalerCount() {
  lock_guard<mutex> lock(_signalerLock);
 
  if (_signalerCount > 0) { _signalerCount--; }
- maybeDestroy();
+ return (_isDestroyed && _signalerCount == 0);
 }
 
 void MVKSignalable::destroy() {
+ if (markDestroyed()) { MVKBaseDeviceObject::destroy(); }
+}
+
+// Marks this object as destroyed, and returns whether the compilation is complete.
+bool MVKSignalable::markDestroyed() {
  lock_guard<mutex> lock(_signalerLock);
 
  _isDestroyed = true;
- maybeDestroy();
-}
-
-void MVKSignalable::maybeDestroy() {
- if (_isDestroyed && _signalerCount == 0) {
- MVKBaseDeviceObject::destroy();
- }
+ return _signalerCount == 0;
 }
 
 
@@ -190,12 +194,25 @@
 #pragma mark Construction
 
 MVKFenceSitter::~MVKFenceSitter() {
- lock_guard<mutex> lock(_lock);
- for (auto& uf : _unsignaledFences) {
+ // Use copy of collection to avoid deadlocks with the fences if lock in place here when removing sitters
+ vector<MVKFence*> ufsCopy;
+ getUnsignaledFences(ufsCopy);
+ for (auto& uf : ufsCopy) {
  uf->removeSitter(this);
  }
 }
 
+// Fills the vector with the collection of unsignaled fences
+void MVKFenceSitter::getUnsignaledFences(vector<MVKFence*>& fences) {
+ fences.clear();
+ fences.reserve(_unsignaledFences.size());
+
+ lock_guard<mutex> lock(_lock);
+ for (auto& uf : _unsignaledFences) {
+ fences.push_back(uf);
+ }
+}
+
 
 #pragma mark -
 #pragma mark Support functions
@@ -231,7 +248,7 @@ VkResult mvkWaitForFences(uint32_t fenceCount,
 // The thread dispatch is needed because even the sync portion of the async Metal compilation methods can take well
 // over a second to return when a compiler failure occurs!
 void MVKMetalCompiler::compile(unique_lock<mutex>& lock, dispatch_block_t block) {
- MVKAssert( _startTime == 0, "%s compile occurred already in this instance. Instances of %s should only be used for a single compile activity.", _compilerType.c_str(), className().c_str());
+ MVKAssert( _startTime == 0, "%s compile occurred already in this instance. Instances of %s should only be used for a single compile activity.", _compilerType.c_str(), getClassName().c_str());
  _startTime = _device->getPerformanceTimestamp();
 
  dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), block);
@@ -254,25 +271,26 @@ VkResult mvkWaitForFences(uint32_t fenceCount,
  setConfigurationResult(mvkNotifyErrorWithText(VK_ERROR_INITIALIZATION_FAILED, "%s compile failed (error code %li):\n%s.", _compilerType.c_str(), (long)_compileError.code, _compileError.localizedDescription.UTF8String));
 }
 
-void MVKMetalCompiler::endCompile(NSError* compileError) {
+// Returns whether the compilation came in late, after the compiler was destroyed.
+bool MVKMetalCompiler::endCompile(NSError* compileError) {
  _compileError = [compileError retain]; // retained
  _isCompileDone = true;
  _blocker.notify_all();
- maybeDestroy();
+ return _isDestroyed;
 }
 
 void MVKMetalCompiler::destroy() {
+ if (markDestroyed()) { MVKBaseDeviceObject::destroy(); }
+}
+
+// Marks this object as destroyed, and returns whether the compilation is complete.
+bool MVKMetalCompiler::markDestroyed() {
  lock_guard<mutex> lock(_completionLock);
 
  _isDestroyed = true;
- maybeDestroy();
+ return _isCompileDone;
 }
 
-void MVKMetalCompiler::maybeDestroy() {
- if (_isDestroyed && _isCompileDone) {
- MVKBaseDeviceObject::destroy();
- }
-}
 
 #pragma mark Construction
 

MoltenVK/MoltenVK/Utility/MVKBaseObject.cpp

@@ -26,7 +26,7 @@ using namespace std;
 #pragma mark -
 #pragma mark MVKBaseObject
 
-string MVKBaseObject::className() {
+string MVKBaseObject::getClassName() {
  int status;
  char* demangled = abi::__cxa_demangle(typeid(*this).name(), 0, 0, &status);
  string clzName = demangled;

MoltenVK/MoltenVK/Utility/MVKBaseObject.h

@@ -35,7 +35,7 @@ class MVKBaseObject {
 public:
 
  /** Returns the name of the class of which this object is an instance. */
- std::string className();
+ std::string getClassName();
 
  /** Destroys this object. Default behaviour simply deletes it. Subclasses may override to delay deletion. */
  virtual void destroy() { delete this; }

MoltenVK/MoltenVK/Utility/MVKOSExtensions.h

@@ -71,3 +71,11 @@ uint64_t mvkRecommendedMaxWorkingSetSize(id<MTLDevice> mtlDevice);
 /** Populate the propertes with info about the GPU represented by the MTLDevice. */
 void mvkPopulateGPUInfo(VkPhysicalDeviceProperties& devProps, id<MTLDevice> mtlDevice);
 
+/** Ensures the block is executed on the main thread. */
+inline void mvkDispatchToMainAndWait(dispatch_block_t block) {
+ if (NSThread.isMainThread) {
+ block();
+ } else {
+ dispatch_sync(dispatch_get_main_queue(), block);
+ }
+}