Should using a singleton PixelShader be a best practice?

Question

In Microsoft's example for how to use the PixelShader they use a singleton. I've seen the same pattern in other places, and here they say

The pixel shader is stored in a private static field _pixelShader. This field is static, because one instance of the compiled shader code is enough for the whole class.

We've seen several memory leak issues when using this pattern. The PixelShader is involved is event handling that don't always get cleared correctly. We had to freeze them, and saw some improvement. We had to manually do some detachments

        // Attach/detach effect as UI element is loaded/unloaded.  This avoids
        // a memory leak in the shader code as described here:
        element.Loaded += (obj, args) =>
        {
            effect.PixelShader = ms_shader;
            element.Effect = effect;
        };
        element.Unloaded += (obj, args) =>
        {
            effect.PixelShader = null;
            element.Effect = null;
        };

And even now under stress there are still memory leaks in that area. Does anyone know if the PixelShader uses heavy resources worth the trouble with using a singleton?

score 4 · Accepted Answer · answered Apr 10 '17 at 19:01

Definitely NO. However, the reason is not with PixelShader itself, but its usage in ShaderEffect.
Almost all implementations of custom Shader Effects for WPF are based on Microsoft's .NET 3.5 samples, which were not updated for .NET 4.0. It was fine to use singleton PixelShader instance for all effects, which supported only ps_2_0 shaders. With .NET 4.0 Microsoft introduced support for ps_3_0 pixel shaders (for GPU-accelerated devices), and with that, they introduced memory leak into ShaderEffect class.
ShaderEffect tracks its PixelShader property and checks that it doesn't use ps_3_0 registers for ps_2_0 bytecode by strong subscription to PixelShader's internal event called _shaderBytecodeChanged. Therefore, singleton PixelShader used by multiple ShaderEffect instances serves as Domination Root for GC and retends all objects which were used with any instance of corresponding ShaderEffect. That is known memory leak, which will not be fixed.
If you would like to use PixelShader as singleton without leaks, then you will have to use runtime hacking: every time PixelShader instance is assigned to PixelShader property of ShaderEffect class, _shaderBytecodeChanged field of PixelShader instance should be manually cleared, e.g.:

var ei = typeof(PixelShader).GetEvent("_shaderBytecodeChanged",
            BindingFlags.Instance | BindingFlags.NonPublic);
var fi = typeof(PixelShader).GetField(ei.Name,
                BindingFlags.Instance | BindingFlags.NonPublic);
fi.SetValue(pixelShader,null);

Of course, these operations can be optimized by generating helper methods in runtime by DynamicMethod infrastructure or similar mechanisms. However, this should be used only for shaders which are definitely ps_2_0 or definitely ps_3_0

What is the preferred way to do this? Make the `PixelShader` non-static? Then it will be duplicated for every instance of the effect, no, though really it is shared an not usually modified? What is the best practice here? — metal, Jan 08 '19 at 20:44
@metal, If you can fix this memory leak (you don't change shader source, you can edit this private field to remove subscription) then you can keep `PixelShader` static, otherwise (partial trust, cannot depent on implementation details) do make `PixelShader` non-static (one per `ShaderEffect`'s instance) to prevent painful memory leaks of UI elements. — Aloraman, Jan 10 '19 at 21:47

Should using a singleton PixelShader be a best practice?

1 Answers1