[performance] Single-lookup SKRuntimeEffectUniforms.Add - #4811
Draft
github-actions[bot] wants to merge 1 commit into
Draft
[performance] Single-lookup SKRuntimeEffectUniforms.Add#4811github-actions[bot] wants to merge 1 commit into
github-actions[bot] wants to merge 1 commit into
Conversation
Replace the Array.IndexOf (O(n) linear scan) plus redundant dictionary indexer in SKRuntimeEffectUniforms.Add with a single Dictionary.TryGetValue. The name->Variable map already answers both the existence check and the fetch, so the linear scan and the extra hash lookup are pure overhead on a per-frame hot path (animated runtime-effect shaders set every uniform each frame). Behaviour is identical: valid names resolve to the same Variable/offset and unknown names still throw ArgumentOutOfRangeException with the same param name and message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
partner/agentic-workflows
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
SKRuntimeEffectUniforms.Addresolved every uniform name twice per call: anArray.IndexOf(names, name)O(n) linear scan (used only for the not-found check) followed by a redundantuniforms[name]hash lookup. Thename -> Variabledictionary already answers both, so this replaces both with a singleDictionary.TryGetValue.Animated runtime-effect shaders assign every uniform each frame via
uniforms["name"] = value(which callsAdd), so this is a real per-frame hot path whose cost grew with the uniform count.Changes
binding/SkiaSharp/SKRuntimeEffect.cs:SKRuntimeEffectUniforms.Addnow uses oneTryGetValueinstead ofArray.IndexOf+ indexer. Method-body-only change; theArgumentOutOfRangeExceptiontype, param name and message for unknown names are unchanged. ABI-safe (no signature/API change).benchmarks/SkiaSharp.Benchmarks/Benchmarks/SKRuntimeEffectUniformsAddBenchmark.cs: New-vs-Old BenchmarkDotNet benchmark mirroringAdd's internals.tests/Tests/SkiaSharp/SKRuntimeEffectTest.cs:AddResolvesUniformsIdenticallyAndRejectsUnknownNamesequivalence regression test.Required skia PR
None.
Areas Affected
Testing
Benchmark (BenchmarkDotNet ShortRun,
MemoryDiagnoser, net10.0 / x64 / Linux):~2x faster at 8 uniforms, ~4x at 32; no allocation change.
Equivalence test:
AddResolvesUniformsIdenticallyAndRejectsUnknownNamesverifies every declared name writes to the correct packed offset and that unknown names throw the sameArgumentOutOfRangeException(same param name + message). Confirmed it FAILS against a deliberately-wrong param name and PASSES on the fix. Full test run via the console host project on net10.0.No rendering change (managed lookup only), so no before/after screenshots apply.
Fixes #4810
Produced by the performance-fixer agentic workflow using the
performance-fixerskill. Numbers are empirically measured (not statically reasoned) on net10.0 / x64 / Linux via BenchmarkDotNet ShortRun; ratios are New/Old and are the portable result (absolute ns are machine-relative). Managed-only change, no ABI impact.