Moved management of shader module creation and lifetime to its own class. #444

softcookiepp · 2026-01-03T22:05:15Z

As the title says, the creation of vk::ShaderModule objects has been moved from kp::Algorithm kp::Module, a new class.
This will make it easier to implement caching of vk::ShaderModule objects, as well as add aditional features for shader module metadata handling in the future.

…e implementation

…e implementation Signed-off-by: softcookiepp <[email protected]>

Signed-off-by: softcookiepp <[email protected]>

axsaucedo

I have done an initial review, seems like still a way to go, there's a few inconsistencies that need to be addressed, as well as caveats on existing approach and coding style. There also doesn't seem to be a fully complete implementation as this suggests an optionally owned resource but currently the PR does not introduce the functionaly to make it optionally owned. We also need to ensure new functionality is tested, especially in contexts where these resources are owned, and particularly assessing the memory management / lifecycle aspect.

axsaucedo · 2026-01-07T09:29:19Z

src/include/kompute/Cache.hpp

@@ -0,0 +1,27 @@
+#pragma once


This file is quite random, what is this here for - remove

axsaucedo · 2026-01-07T09:41:54Z

src/include/kompute/Shader.hpp

+	/*
+	 * getter for mShaderModule
+	 */
+	vk::ShaderModule& getShaderModule() { return mShaderModule; }


Should be const and should not return a mutable reference

axsaucedo · 2026-01-07T09:42:13Z

src/include/kompute/Shader.hpp

+public:
+
+	/*
+	 * Constructor accepting a device and a SPIR-V binary


Comment conventions should align to rest of codebase - see manager.hpp

axsaucedo · 2026-01-07T09:43:13Z

src/include/kompute/Shader.hpp

+	// the vulkan device; not owned by this object
+	std::weak_ptr<vk::Device> mDevice;
+
+	// the shader module handle


Superfluous comment, see other classes for examples

needs clarity on ownerhsip as this is sometimes owned resource

axsaucedo · 2026-01-07T09:44:34Z

src/include/kompute/Shader.hpp

+ * Wrapper for Vulkan's shader modules.
+ * The purpose of this is to manage the module lifetime, while
+ * building the groundwork for easily integrating things like
+ * SPIR-V reflection and multiple entry points in the future.


Rewrite as description of class as of now not for future / groundwork

axsaucedo · 2026-01-07T09:49:25Z

src/include/kompute/Shader.hpp

+ * building the groundwork for easily integrating things like
+ * SPIR-V reflection and multiple entry points in the future.
+ */
+class Module : public std::enable_shared_from_this<Module>


Naming convention here is not consistent, should be Shader as per file - module is a generic keyword

axsaucedo · 2026-01-07T09:53:06Z

src/include/kompute/Algorithm.hpp

    bool mFreeDescriptorSet = false;
-    std::shared_ptr<vk::ShaderModule> mShaderModule;
-    bool mFreeShaderModule = false;
+	std::shared_ptr<Module> mModule = nullptr;


This is suggested as optionally owned resource but it's not handled as such, follow the same convention as per the rest of the optionally owned resources

axsaucedo · 2026-01-07T09:55:03Z

src/Shader.cpp

+				 spv.size());
+	vk::ShaderModuleCreateInfo shaderModuleInfo(vk::ShaderModuleCreateFlags(),
+		sizeof(uint32_t) * spv.size(), spv.data());
+	this->mDevice.lock()->createShaderModule(


Currently we don't use device.lock throughout the codebase, let's ensure consitency

axsaucedo · 2026-01-07T09:56:23Z

src/include/kompute/Shader.hpp

+class Module : public std::enable_shared_from_this<Module>
+{
+	// the vulkan device; not owned by this object
+	std::weak_ptr<vk::Device> mDevice;


Device is not a weakptr as we need to ensure that this resource doesnt outlive the lifecylce by storing as strong reference and/or ensuring this is handled correctly

axsaucedo · 2026-01-07T09:58:15Z

src/Shader.cpp

+{
+	KP_LOG_DEBUG("Kompute Module destructor started");
+	KP_LOG_DEBUG("Kompute Module Destroying shader module");
+	if (!mDevice.expired() )


Ensure consistency with other classes, including coding style

softcookiepp · 2026-01-08T18:55:15Z

Alright, that should cover everything.

softcookiepp and others added 4 commits January 3, 2026 14:02

moved shader module creation to its own class in preparation for cach…

c75c161

…e implementation

moved shader module creation to its own class in preparation for cach…

74841ce

…e implementation Signed-off-by: softcookiepp <[email protected]>

Merge branch 'master' of github.com:softcookiepp/kompute

5d737b9

Signed-off-by: softcookiepp <[email protected]>

Removed Cache.cpp, as it will not be used.

6fdc886

Signed-off-by: softcookiepp <[email protected]>

axsaucedo requested changes Jan 8, 2026

View reviewed changes

softcookiepp added 2 commits January 8, 2026 10:24

completely forgot to remove Cache.hpp, as it is not used

acba5b8

as you wish

64edc39

softcookiepp requested a review from axsaucedo January 8, 2026 19:43

Moved management of shader module creation and lifetime to its own class. #444

Are you sure you want to change the base?

Moved management of shader module creation and lifetime to its own class. #444

Conversation

softcookiepp commented Jan 3, 2026

Uh oh!

axsaucedo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

softcookiepp commented Jan 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants