From 57726924fa05c9d498ac9b8d1a2fe56191a46cd0 Mon Sep 17 00:00:00 2001 From: Carol Payne Date: Sun, 2 Aug 2026 17:31:25 -0700 Subject: [PATCH 1/2] Hold constant references to array uniforms in the Metal class wrapper The MSL class wrapper gave the generated struct owning copies of every array uniform, with a constructor that copied the used range out of constant memory into per-thread private memory and then zero-filled the remainder up to the full declared capacity. The struct is instantiated once per shader invocation, so a compute shader dispatched one thread per pixel paid that copy per pixel: for a dynamic GradingRGBCurve that is 8 + 120 + 8 + 360 = 496 private-memory writes for every pixel, regardless of how many knots the curve actually has. Point at the constant memory instead. The array declarations arrive from the ops shared with the other GPU languages, so the wrapper rewrites them to constant pointers rather than changing the ops; it already knows which parameters are arrays. Indexing syntax is unchanged, so every read site in the generated shader is untouched, and the generated function signature is unchanged so existing MSL consumers keep working. Dropping the zero-fill does not change any result. Reads of knots/coefs are bounded by the offset taken from the offsets array, and the offsets arrays are always fully populated -- GetNumOffsetValues() is a compile-time constant equal to the declared capacity (8 for RGB, 16 for hue) -- so no read past the used range was ever possible. Measured with a Metal compute kernel over an RGBA16F 4K UHD frame, one thread per pixel, output texels bit-identical before and after: rgb curve only 33.45 ms -> at the read+write bandwidth floor luma curve only 35.65 ms -> at the floor hue curve only 35.46 ms -> at the floor rgb + luma + hue 115.28 ms -> at the floor GradingRGBCurve and GradingHueCurve are the only ops that declare array uniforms; everything else passes scalars or textures and is unaffected. MetalSupport9 pins the generated text, so its expected shader is updated. Three assertions are added alongside it stating the no-copy invariant on its own, so a future refresh of the expected text cannot quietly restore the per-pixel copy. Signed-off-by: Carol Payne --- src/OpenColorIO/GpuShaderClassWrapper.cpp | 66 ++++++++++++----------- src/OpenColorIO/GpuShaderClassWrapper.h | 1 + tests/cpu/GpuShader_tests.cpp | 59 ++++++++------------ 3 files changed, 58 insertions(+), 68 deletions(-) diff --git a/src/OpenColorIO/GpuShaderClassWrapper.cpp b/src/OpenColorIO/GpuShaderClassWrapper.cpp index 675bd47163..eed6912eed 100644 --- a/src/OpenColorIO/GpuShaderClassWrapper.cpp +++ b/src/OpenColorIO/GpuShaderClassWrapper.cpp @@ -181,36 +181,10 @@ std::string MetalShaderClassWrapper::generateClassWrapperHeader(GpuShaderText& k kw.indent(); for(const auto& param : m_functionParameters) { - size_t openAngledBracketPos = param.m_name.find('['); - if(!param.m_isArray) - { - kw.newLine() << "this->" << param.m_name << " = " << param.m_name << ";"; - } - else - { - size_t closeAngledBracketPos = param.m_name.find(']'); - std::string variableName = param.m_name.substr(0, openAngledBracketPos); - - kw.newLine() << "for(int i = 0; i < " - << GetArrayLengthVariableName(variableName) - << "; ++i)"; - kw.newLine() << "{"; - kw.indent(); - kw.newLine() << "this->" << variableName << "[i] = " << variableName << "[i];"; - kw.dedent(); - kw.newLine() << "}"; - - kw.newLine() << "for(int i = " - << GetArrayLengthVariableName(variableName) - << "; i < " - << param.m_name.substr(openAngledBracketPos+1, closeAngledBracketPos-openAngledBracketPos-1) - << "; ++i)"; - kw.newLine() << "{"; - kw.indent(); - kw.newLine() << "this->" << variableName << "[i] = 0;"; - kw.dedent(); - kw.newLine() << "}"; - } + // Array members are pointers into constant memory (see rewriteArrayDeclarations) so they + // are bound rather than copied, using the parameter name without its array size. + std::string variableName = param.m_name.substr(0, param.m_name.find('[')); + kw.newLine() << "this->" << variableName << " = " << variableName << ";"; } kw.dedent(); kw.newLine() <<"}"; @@ -378,6 +352,36 @@ void MetalShaderClassWrapper::prepareClassWrapper(const std::string& resourcePre extractFunctionParameters(originalHeader); } +std::string MetalShaderClassWrapper::rewriteArrayDeclarations(const std::string& declarations) const +{ + // The uniform declarations are shared with the other GPU languages so arrays arrive here as + // fixed-size members i.e. 'float name[120];'. Owning them would make the constructor copy the + // whole array out of constant memory into per-thread memory, which is prohibitive as the + // struct is typically instantiated once per pixel. Hold a pointer to the constant memory + // instead; every read site is unchanged as the indexing syntax is the same. + // + // Note that nothing zero-fills the members between the array length and the declared capacity + // any more. An op declaring an array uniform must therefore keep its reads below the length it + // reports, as only that many elements are uploaded: a read beyond it now runs past the + // uploaded data instead of returning zero. + std::string rewritten = declarations; + for(const auto& param : m_functionParameters) + { + if(!param.m_isArray) + continue; + + const std::string declaration = param.m_type + " " + param.m_name + ";"; + const size_t pos = rewritten.find(declaration); + if(pos != std::string::npos) + { + rewritten.replace(pos, declaration.size(), + "constant " + param.m_type + "* " + + param.m_name.substr(0, param.m_name.find('[')) + ";"); + } + } + return rewritten; +} + std::string MetalShaderClassWrapper::getClassWrapperHeader(const std::string& originalHeader) { GpuShaderText st(GPU_LANGUAGE_MSL_2_0); @@ -388,7 +392,7 @@ std::string MetalShaderClassWrapper::getClassWrapperHeader(const std::string& or std::string classWrapHeader = "\n// Declaration of class wrapper\n\n"; classWrapHeader += st.string(); - return classWrapHeader + originalHeader; + return classWrapHeader + rewriteArrayDeclarations(originalHeader); } std::string MetalShaderClassWrapper::getClassWrapperFooter(const std::string& originalFooter) diff --git a/src/OpenColorIO/GpuShaderClassWrapper.h b/src/OpenColorIO/GpuShaderClassWrapper.h index cbe01b3545..5a18b4b806 100644 --- a/src/OpenColorIO/GpuShaderClassWrapper.h +++ b/src/OpenColorIO/GpuShaderClassWrapper.h @@ -120,6 +120,7 @@ class MetalShaderClassWrapper : public GpuShaderClassWrapper static std::string getClassWrapperName(const std::string &resourcePrefix, const std::string &functionName); void extractFunctionParameters(const std::string& declaration); + std::string rewriteArrayDeclarations(const std::string& declarations) const; std::string generateClassWrapperHeader(GpuShaderText& st) const; std::string generateClassWrapperFooter(GpuShaderText& st, const std::string &ocioFunctionName) const; diff --git a/tests/cpu/GpuShader_tests.cpp b/tests/cpu/GpuShader_tests.cpp index eb2bc09dc8..4b7acf8069 100644 --- a/tests/cpu/GpuShader_tests.cpp +++ b/tests/cpu/GpuShader_tests.cpp @@ -1215,48 +1215,20 @@ ocioOCIOMain( , bool ocio_grading_rgbcurve_localBypass ) { - for(int i = 0; i < ocio_grading_rgbcurve_knotsOffsets_count; ++i) - { - this->ocio_grading_rgbcurve_knotsOffsets[i] = ocio_grading_rgbcurve_knotsOffsets[i]; - } - for(int i = ocio_grading_rgbcurve_knotsOffsets_count; i < 8; ++i) - { - this->ocio_grading_rgbcurve_knotsOffsets[i] = 0; - } - for(int i = 0; i < ocio_grading_rgbcurve_knots_count; ++i) - { - this->ocio_grading_rgbcurve_knots[i] = ocio_grading_rgbcurve_knots[i]; - } - for(int i = ocio_grading_rgbcurve_knots_count; i < 120; ++i) - { - this->ocio_grading_rgbcurve_knots[i] = 0; - } - for(int i = 0; i < ocio_grading_rgbcurve_coefsOffsets_count; ++i) - { - this->ocio_grading_rgbcurve_coefsOffsets[i] = ocio_grading_rgbcurve_coefsOffsets[i]; - } - for(int i = ocio_grading_rgbcurve_coefsOffsets_count; i < 8; ++i) - { - this->ocio_grading_rgbcurve_coefsOffsets[i] = 0; - } - for(int i = 0; i < ocio_grading_rgbcurve_coefs_count; ++i) - { - this->ocio_grading_rgbcurve_coefs[i] = ocio_grading_rgbcurve_coefs[i]; - } - for(int i = ocio_grading_rgbcurve_coefs_count; i < 360; ++i) - { - this->ocio_grading_rgbcurve_coefs[i] = 0; - } + this->ocio_grading_rgbcurve_knotsOffsets = ocio_grading_rgbcurve_knotsOffsets; + this->ocio_grading_rgbcurve_knots = ocio_grading_rgbcurve_knots; + this->ocio_grading_rgbcurve_coefsOffsets = ocio_grading_rgbcurve_coefsOffsets; + this->ocio_grading_rgbcurve_coefs = ocio_grading_rgbcurve_coefs; this->ocio_grading_rgbcurve_localBypass = ocio_grading_rgbcurve_localBypass; } // Declaration of all variables -int ocio_grading_rgbcurve_knotsOffsets[8]; -float ocio_grading_rgbcurve_knots[120]; -int ocio_grading_rgbcurve_coefsOffsets[8]; -float ocio_grading_rgbcurve_coefs[360]; +constant int* ocio_grading_rgbcurve_knotsOffsets; +constant float* ocio_grading_rgbcurve_knots; +constant int* ocio_grading_rgbcurve_coefsOffsets; +constant float* ocio_grading_rgbcurve_coefs; bool ocio_grading_rgbcurve_localBypass; @@ -1361,8 +1333,21 @@ float4 OCIOMain( ).OCIOMain(inPixel); } )" }; - + OCIO_CHECK_EQUAL(expected, text); + + // The struct is instantiated once per invocation, which for a compute shader + // dispatched one thread per pixel means once per pixel. Owning the array + // uniforms rather than pointing at them therefore copies the whole declared + // capacity into per-thread memory for every pixel, which dominated the cost + // of any curve op. These checks state that invariant on its own so a future + // update of the expected text above cannot quietly restore the copy. + OCIO_CHECK_ASSERT(text.find("constant float* ocio_grading_rgbcurve_knots;") + != std::string::npos); + OCIO_CHECK_ASSERT(text.find("this->ocio_grading_rgbcurve_knots = " + "ocio_grading_rgbcurve_knots;") != std::string::npos); + OCIO_CHECK_ASSERT(text.find("this->ocio_grading_rgbcurve_knots[i]") + == std::string::npos); } OCIO_ADD_TEST(GpuShader, VulkanSupport) From addb28341d4078479243d76eec4fd3bd76f0d3f4 Mon Sep 17 00:00:00 2001 From: Carol Payne Date: Fri, 11 Sep 2026 08:20:29 -0700 Subject: [PATCH 2/2] Address review: state the per-invocation mechanism, drop the shader-text asserts Signed-off-by: Carol Payne --- src/OpenColorIO/GpuShaderClassWrapper.cpp | 8 +++++--- tests/cpu/GpuShader_tests.cpp | 13 ------------- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/src/OpenColorIO/GpuShaderClassWrapper.cpp b/src/OpenColorIO/GpuShaderClassWrapper.cpp index eed6912eed..9584cc6ed1 100644 --- a/src/OpenColorIO/GpuShaderClassWrapper.cpp +++ b/src/OpenColorIO/GpuShaderClassWrapper.cpp @@ -356,9 +356,11 @@ std::string MetalShaderClassWrapper::rewriteArrayDeclarations(const std::string& { // The uniform declarations are shared with the other GPU languages so arrays arrive here as // fixed-size members i.e. 'float name[120];'. Owning them would make the constructor copy the - // whole array out of constant memory into per-thread memory, which is prohibitive as the - // struct is typically instantiated once per pixel. Hold a pointer to the constant memory - // instead; every read site is unchanged as the indexing syntax is the same. + // whole array out of constant memory into per-thread memory. The struct is constructed inside + // the generated OCIOMain(), which takes and returns a single pixel, so that copy is paid once + // per invocation: once per pixel for a fragment shader or a one-thread-per-pixel kernel. Hold + // a pointer to the constant memory instead; every read site is unchanged as the indexing + // syntax is the same. // // Note that nothing zero-fills the members between the array length and the declared capacity // any more. An op declaring an array uniform must therefore keep its reads below the length it diff --git a/tests/cpu/GpuShader_tests.cpp b/tests/cpu/GpuShader_tests.cpp index 4b7acf8069..3d7708abb7 100644 --- a/tests/cpu/GpuShader_tests.cpp +++ b/tests/cpu/GpuShader_tests.cpp @@ -1335,19 +1335,6 @@ float4 OCIOMain( )" }; OCIO_CHECK_EQUAL(expected, text); - - // The struct is instantiated once per invocation, which for a compute shader - // dispatched one thread per pixel means once per pixel. Owning the array - // uniforms rather than pointing at them therefore copies the whole declared - // capacity into per-thread memory for every pixel, which dominated the cost - // of any curve op. These checks state that invariant on its own so a future - // update of the expected text above cannot quietly restore the copy. - OCIO_CHECK_ASSERT(text.find("constant float* ocio_grading_rgbcurve_knots;") - != std::string::npos); - OCIO_CHECK_ASSERT(text.find("this->ocio_grading_rgbcurve_knots = " - "ocio_grading_rgbcurve_knots;") != std::string::npos); - OCIO_CHECK_ASSERT(text.find("this->ocio_grading_rgbcurve_knots[i]") - == std::string::npos); } OCIO_ADD_TEST(GpuShader, VulkanSupport)