From d3934f5b07ccbdb8e36e733c12d00f4f53811097 Mon Sep 17 00:00:00 2001
From: Kaylee Lubick <kjlubick@google.com>
Date: Mon, 27 Apr 2026 18:23:31 +0000
Subject: [PATCH] Make local optimized copy of program when creating SkRP version
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

As per the linked bug, there could be a problem if the gpu backend
tried to use a compiled runtime effect at the same time as the
cpu backend made its first call to getRPProgram(). This could result
in the fBaselineProgram being mutated out from under the gpu backend's
version.

This makes an optimized copy of the existing program to then convert
to SkRasterPipeline. By setting optimize to true and a non-zero
inlining limit, compiler.convertProgram will call the inliner and
remove unused functions (as well as other things like unused
local/global variables).

To prevent further mutation of fBaseProgram, I made it be pointer
to const (I'd initially been puzzled how a const getRPProgram could
be mutating it, but only the pointer was const).

Performance change looks negligible (parsing is pretty quick):
```
$ out/Release/nanobench_baseline --match sksl_skrp
Timer overhead: 23.5ns
curr/maxrss	loops	min	median	mean	max	stddev	samples   	config	bench
  92/90  MB	3	5.35µs	5.38µs	5.38µs	5.45µs	0%	█▅▁▃▃▄▄▂▃▃	nonrendering	sksl_skrp_tiny
  92/90  MB	3	14.7µs	14.9µs	15.1µs	17.5µs	6%	█▂▁▁▁▁▁▁▁▁	nonrendering	sksl_skrp_small
  92/90  MB	1	125µs	132µs	135µs	164µs	9%	█▅▃▂▂▁▂▁▁▄	nonrendering	sksl_skrp_medium
  92/90  MB	1	321µs	338µs	339µs	364µs	5%	█▆▃▂█▄▂▁▅▂	nonrendering	sksl_skrp_large

$ out/Release/nanobench_with_change --match sksl_skrp
Timer overhead: 23.5ns
curr/maxrss	loops	min	median	mean	max	stddev	samples   	config	bench
  92/90  MB	3	5.62µs	5.64µs	5.64µs	5.69µs	0%	█▂▄▄▃▃▄▁▅▃	nonrendering	sksl_skrp_tiny
  92/90  MB	4	14.8µs	14.9µs	15.3µs	17.1µs	5%	█▂▁▁▅▁▁▁▁▁	nonrendering	sksl_skrp_small
  92/90  MB	2	125µs	130µs	131µs	154µs	7%	█▃▂▂▂▁▁▂▁▃	nonrendering	sksl_skrp_medium
  92/90  MB	1	322µs	344µs	342µs	369µs	5%	█▆▅▃▂▁▇▄▂▃	nonrendering	sksl_skrp_large
```

Bug: https://issues.chromium.org/issues/498989348
Change-Id: Idf32b817f5d3de823bb95f588e69e4e8a58ead80
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1219876
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Kaylee Lubick <kjlubick@google.com>
---
 include/effects/SkRuntimeEffect.h |  4 +--
 src/core/SkRuntimeEffect.cpp      | 45 ++++++++++++++++++++++---------
 2 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/include/effects/SkRuntimeEffect.h b/include/effects/SkRuntimeEffect.h
index e36f92f95d..a53e72139e 100644
--- a/include/effects/SkRuntimeEffect.h
+++ b/include/effects/SkRuntimeEffect.h
@@ -324,8 +324,8 @@ private:
     uint32_t fStableKey = 0;
     SkString fName;
 
-    std::unique_ptr<SkSL::Program> fBaseProgram;
-    std::unique_ptr<SkSL::RP::Program> fRPProgram;
+    std::unique_ptr<const SkSL::Program> fBaseProgram;
+    std::unique_ptr<const SkSL::RP::Program> fRPProgram;
     mutable SkOnce fCompileRPProgramOnce;
     const SkSL::FunctionDefinition& fMain;
     std::vector<Uniform> fUniforms;
diff --git a/src/core/SkRuntimeEffect.cpp b/src/core/SkRuntimeEffect.cpp
index ee4bfcb6fe..be07904c2f 100644
--- a/src/core/SkRuntimeEffect.cpp
+++ b/src/core/SkRuntimeEffect.cpp
@@ -222,30 +222,48 @@ const SkSL::RP::Program* SkRuntimeEffect::getRPProgram(SkSL::DebugTracePriv* deb
     fCompileRPProgramOnce([&] {
         // We generally do not run the inliner when an SkRuntimeEffect program is initially created,
         // because the final compile to native shader code will do this. However, in SkRP, there's
-        // no additional compilation occurring, so we need to manually inline here if we want the
-        // performance boost of inlining.
-        if (!(fFlags & kDisableOptimization_Flag)) {
+        // no additional compilation occurring, so we need to optimize/inline here if we want the
+        // performance boost of inlining. Since fBaseProgram is a shared (const) object, we can't
+        // mutate it in-place (e.g. calling compiler.runInliner). If optimization is neccesary,
+        // we re-compile the program from source with inlining and optimization enabled to get a
+        // freshly optimized copy (it's pretty cheap to re-compile and there's no easy way to copy
+        // an SkSL::Program).
+        const SkSL::Program* programToUse = fBaseProgram.get();
+        const SkSL::FunctionDefinition* mainToUse = &fMain;
+
+        std::unique_ptr<SkSL::Program> optimizedCopy;
+        bool shouldOptimize = !(fFlags & kDisableOptimization_Flag);
+        SkSL::ProgramSettings settings = fBaseProgram->fConfig->fSettings;
+        bool needsOptimization = !settings.fOptimize ||
+                                  settings.fInlineThreshold < SkSL::kDefaultInlineThreshold;
+        if (shouldOptimize && needsOptimization) {
             SkSL::Compiler compiler;
-            fBaseProgram->fConfig->fSettings.fInlineThreshold = SkSL::kDefaultInlineThreshold;
-            compiler.runInliner(*fBaseProgram);
-
-            // After inlining, the program is likely to have dead functions left behind.
-            while (SkSL::Transform::EliminateDeadFunctions(*fBaseProgram)) {
-                // Removing dead functions may cause more functions to become unreferenced.
+            settings.fOptimize = true;
+            settings.fInlineThreshold = SkSL::kDefaultInlineThreshold;
+            optimizedCopy = compiler.convertProgram(
+                    fBaseProgram->fConfig->fKind, *fBaseProgram->fSource, settings);
+            SkASSERT(optimizedCopy);
+            if (optimizedCopy) {
+                const auto* mainDecl = optimizedCopy->getFunction("main");
+                SkASSERT(mainDecl);
+                if (mainDecl) {
+                    programToUse = optimizedCopy.get();
+                    mainToUse = mainDecl->definition();
+                }
             }
         }
 
         SkSL::DebugTracePriv tempDebugTrace;
         if (debugTrace) {
             const_cast<SkRuntimeEffect*>(this)->fRPProgram = MakeRasterPipelineProgram(
-                    *fBaseProgram, fMain, debugTrace, /*writeTraceOps=*/true);
+                    *programToUse, *mainToUse, debugTrace, /*writeTraceOps=*/true);
         } else if (kRPEnableLiveTrace) {
             debugTrace = &tempDebugTrace;
             const_cast<SkRuntimeEffect*>(this)->fRPProgram = MakeRasterPipelineProgram(
-                    *fBaseProgram, fMain, debugTrace, /*writeTraceOps=*/false);
+                    *programToUse, *mainToUse, debugTrace, /*writeTraceOps=*/false);
         } else {
             const_cast<SkRuntimeEffect*>(this)->fRPProgram = MakeRasterPipelineProgram(
-                    *fBaseProgram, fMain, /*debugTrace=*/nullptr, /*writeTraceOps=*/false);
+                    *programToUse, *mainToUse, /*debugTrace=*/nullptr, /*writeTraceOps=*/false);
         }
 
         if (kRPEnableLiveTrace) {
@@ -450,8 +468,9 @@ void SkRuntimeEffectPriv::WriteChildEffects(
 }
 
 SkSL::ProgramSettings SkRuntimeEffect::MakeSettings(const Options& options) {
+    constexpr int kDisableSKSLInlining = 0;
     SkSL::ProgramSettings settings;
-    settings.fInlineThreshold = 0;
+    settings.fInlineThreshold = kDisableSKSLInlining;
     settings.fForceNoInline = options.forceUnoptimized;
     settings.fOptimize = !options.forceUnoptimized;
     settings.fMaxVersionAllowed = options.maxVersionAllowed;
-- 
2.43.0

