From b403912da8ea392073ee8623cfd6ec31a7ff6165 Mon Sep 17 00:00:00 2001 From: Peter Gagliardi Date: Thu, 25 Aug 2022 11:17:44 -0400 Subject: [PATCH 1/3] Make the ShaderCache key simpler to compute --- Source/Renderer/ShaderCache.js | 31 ++++++++++++++++++++++--------- Source/Renderer/ShaderSource.js | 11 +++++++++++ 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/Source/Renderer/ShaderCache.js b/Source/Renderer/ShaderCache.js index 85ee8787de14..3b6f91e1551d 100644 --- a/Source/Renderer/ShaderCache.js +++ b/Source/Renderer/ShaderCache.js @@ -56,6 +56,11 @@ ShaderCache.prototype.replaceShaderProgram = function (options) { return this.getShaderProgram(options); }; +function toSortedJson(dictionary) { + const sortedKeys = Object.keys(dictionary).sort(); + return JSON.stringify(dictionary, sortedKeys); +} + /** * Returns a shader program from the cache, or creates and caches a new shader program, * given the GLSL vertex and fragment shader source and attribute locations. @@ -87,17 +92,17 @@ ShaderCache.prototype.getShaderProgram = function (options) { }); } - const vertexShaderText = vertexShaderSource.createCombinedVertexShader( - this._context - ); - const fragmentShaderText = fragmentShaderSource.createCombinedFragmentShader( - this._context - ); + // Since ShaderSource.createCombinedXxxShader() can be expensive, use a + // simpler key for caching. + const vertexShaderKey = vertexShaderSource.getCacheKey(); + const fragmentShaderKey = fragmentShaderSource.getCacheKey(); + // Sort the keys in the JSON to ensure a consistent order + const attributeLocationKey = defined(attributeLocations) + ? toSortedJson(attributeLocations) + : ""; + const keyword = `${vertexShaderKey}:${fragmentShaderKey}:${attributeLocationKey}`; - const keyword = - vertexShaderText + fragmentShaderText + JSON.stringify(attributeLocations); let cachedShader; - if (defined(this._shaders[keyword])) { cachedShader = this._shaders[keyword]; @@ -105,6 +110,14 @@ ShaderCache.prototype.getShaderProgram = function (options) { delete this._shadersToRelease[keyword]; } else { const context = this._context; + + const vertexShaderText = vertexShaderSource.createCombinedVertexShader( + this._context + ); + const fragmentShaderText = fragmentShaderSource.createCombinedFragmentShader( + this._context + ); + const shaderProgram = new ShaderProgram({ gl: context._gl, logShaderCompilation: context.logShaderCompilation, diff --git a/Source/Renderer/ShaderSource.js b/Source/Renderer/ShaderSource.js index 53cb05f81dbb..a8ba2afa8a1a 100644 --- a/Source/Renderer/ShaderSource.js +++ b/Source/Renderer/ShaderSource.js @@ -358,6 +358,17 @@ ShaderSource.replaceMain = function (source, renamedMain) { return source.replace(/void\s+main\s*\(\s*(?:void)?\s*\)/g, renamedMain); }; +ShaderSource.prototype.getCacheKey = function () { + // Sort defines + const sortedDefines = this.defines.slice().sort(); + const definesKey = sortedDefines.join(","); + const pickKey = this.pickColorQualifier; + const builtinsKey = this.includeBuiltIns; + const sourcesKey = this.sources.join("\n"); + + return `${definesKey}:${pickKey}:${builtinsKey}:${sourcesKey}`; +}; + /** * Create a single string containing the full, combined vertex shader with all dependencies and defines. * From a782b448c0387d57a6b1096f676e90a112ba30bb Mon Sep 17 00:00:00 2001 From: Peter Gagliardi Date: Thu, 25 Aug 2022 16:24:36 -0400 Subject: [PATCH 2/3] Add unit tests --- Source/Renderer/ShaderSource.js | 11 +++- Specs/Renderer/ShaderCacheSpec.js | 82 +++++++++++++++++++++++++++++- Specs/Renderer/ShaderSourceSpec.js | 42 +++++++++++++++ 3 files changed, 133 insertions(+), 2 deletions(-) diff --git a/Source/Renderer/ShaderSource.js b/Source/Renderer/ShaderSource.js index a8ba2afa8a1a..330c604170e4 100644 --- a/Source/Renderer/ShaderSource.js +++ b/Source/Renderer/ShaderSource.js @@ -358,8 +358,17 @@ ShaderSource.replaceMain = function (source, renamedMain) { return source.replace(/void\s+main\s*\(\s*(?:void)?\s*\)/g, renamedMain); }; +/** + * Since {@link ShaderSource#createCombinedVertexShader} and + * {@link ShaderSource#createCombinedFragmentShader} are both expensive to + * compute, create a simpler string key for lookups in the {@link ShaderCache}. + * + * @returns {String} A key for identifying this shader + * + * @private + */ ShaderSource.prototype.getCacheKey = function () { - // Sort defines + // Sort defines to make the key comparison deterministic const sortedDefines = this.defines.slice().sort(); const definesKey = sortedDefines.join(","); const pickKey = this.pickColorQualifier; diff --git a/Specs/Renderer/ShaderCacheSpec.js b/Specs/Renderer/ShaderCacheSpec.js index 37c4f8085ac5..e03ee4ec0cef 100644 --- a/Specs/Renderer/ShaderCacheSpec.js +++ b/Specs/Renderer/ShaderCacheSpec.js @@ -1,4 +1,4 @@ -import { ShaderCache } from "../../Source/Cesium.js"; +import { ShaderCache, ShaderSource } from "../../Source/Cesium.js"; import createContext from "../createContext.js"; describe( @@ -72,6 +72,17 @@ describe( "attribute vec4 position; void main() { gl_Position = position; }"; const fs = "void main() { gl_FragColor = vec4(1.0); }"; + // These functions can be expensive for large shaders, so they should + // only be called the first time a shader is created. + spyOn( + ShaderSource.prototype, + "createCombinedVertexShader" + ).and.callThrough(); + spyOn( + ShaderSource.prototype, + "createCombinedFragmentShader" + ).and.callThrough(); + const cache = new ShaderCache(context); const sp = cache.getShaderProgram({ vertexShaderSource: vs, @@ -92,6 +103,75 @@ describe( expect(sp._cachedShader.count).toEqual(2); expect(cache.numberOfShaders).toEqual(1); + expect( + ShaderSource.prototype.createCombinedVertexShader + ).toHaveBeenCalledTimes(1); + expect( + ShaderSource.prototype.createCombinedFragmentShader + ).toHaveBeenCalledTimes(1); + + sp.destroy(); + sp2.destroy(); + cache.destroyReleasedShaderPrograms(); + + expect(sp.isDestroyed()).toEqual(true); + expect(cache.numberOfShaders).toEqual(0); + + cache.destroy(); + }); + + it("cache handles unordered attributeLocations dictionary", function () { + const vs = + "attribute vec4 position; void main() { gl_Position = position; }"; + const fs = "void main() { gl_FragColor = vec4(1.0); }"; + + // Create a case where JSON.stringify(x) may produce two different results + // without sorting the keys + const attributeLocations = { + position: 0, + normal: 1, + color: 2, + }; + const attributeLocationsReordered = { + color: 2, + position: 0, + normal: 1, + }; + + // These functions can be expensive for large shaders, so they should + // only be called the first time a shader is created. + spyOn( + ShaderSource.prototype, + "createCombinedVertexShader" + ).and.callThrough(); + spyOn( + ShaderSource.prototype, + "createCombinedFragmentShader" + ).and.callThrough(); + + const cache = new ShaderCache(context); + const sp = cache.getShaderProgram({ + vertexShaderSource: vs, + fragmentShaderSource: fs, + attributeLocations: attributeLocations, + }); + const sp2 = cache.getShaderProgram({ + vertexShaderSource: vs, + fragmentShaderSource: fs, + attributeLocations: attributeLocationsReordered, + }); + + expect(sp).toBe(sp2); + expect(sp._cachedShader.count).toEqual(2); + expect(cache.numberOfShaders).toEqual(1); + + expect( + ShaderSource.prototype.createCombinedVertexShader + ).toHaveBeenCalledTimes(1); + expect( + ShaderSource.prototype.createCombinedFragmentShader + ).toHaveBeenCalledTimes(1); + sp.destroy(); sp2.destroy(); cache.destroyReleasedShaderPrograms(); diff --git a/Specs/Renderer/ShaderSourceSpec.js b/Specs/Renderer/ShaderSourceSpec.js index e8d9c162b15a..1e3e06eaa205 100644 --- a/Specs/Renderer/ShaderSourceSpec.js +++ b/Specs/Renderer/ShaderSourceSpec.js @@ -91,4 +91,46 @@ describe("Renderer/ShaderSource", function () { expect(clone.pickColorQualifier).toEqual(source.pickColorQualifier); expect(clone.includeBuiltIns).toEqual(source.includeBuiltIns); }); + + it("creates cache key for empty shader", function () { + const source = new ShaderSource(); + expect(source.getCacheKey()).toBe(":undefined:true:"); + }); + + it("creates cache key", function () { + const source = new ShaderSource({ + defines: ["A", "B", "C"], + sources: ["void main() { gl_FragColor = vec4(1.0); }"], + pickColorQualifier: "varying", + includeBuiltIns: false, + }); + + expect(source.getCacheKey()).toBe( + "A,B,C:varying:false:void main() { gl_FragColor = vec4(1.0); }" + ); + }); + + it("uses sorted list of defines in cache key", function () { + const defines1 = ["A", "B", "C"]; + const defines2 = ["B", "C", "A"]; + + const source1 = new ShaderSource({ defines: defines1 }); + const source2 = new ShaderSource({ defines: defines2 }); + const key1 = source1.getCacheKey(); + expect(key1).toBe(source2.getCacheKey()); + expect(key1).toBe("A,B,C:undefined:true:"); + }); + + it("cache key includes all sources", function () { + const source = new ShaderSource({ + sources: [ + "vec4 getColor() { return vec4(1.0, 0.0, 0.0, 1.0); }", + "void main() { gl_fragColor = getColor(); }", + ], + }); + + expect(source.getCacheKey()).toBe( + ":undefined:true:vec4 getColor() { return vec4(1.0, 0.0, 0.0, 1.0); }\nvoid main() { gl_fragColor = getColor(); }" + ); + }); }); From 0fef94489af986d0638469f63feefabed3be9455 Mon Sep 17 00:00:00 2001 From: Peter Gagliardi Date: Tue, 30 Aug 2022 11:46:21 -0400 Subject: [PATCH 3/3] PR feedback --- Source/Renderer/ShaderCache.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Source/Renderer/ShaderCache.js b/Source/Renderer/ShaderCache.js index 3b6f91e1551d..b7addaf09d69 100644 --- a/Source/Renderer/ShaderCache.js +++ b/Source/Renderer/ShaderCache.js @@ -93,7 +93,8 @@ ShaderCache.prototype.getShaderProgram = function (options) { } // Since ShaderSource.createCombinedXxxShader() can be expensive, use a - // simpler key for caching. + // simpler key for caching. This way, the function does not have to be called + // for each cache lookup. const vertexShaderKey = vertexShaderSource.getCacheKey(); const fragmentShaderKey = fragmentShaderSource.getCacheKey(); // Sort the keys in the JSON to ensure a consistent order @@ -112,10 +113,10 @@ ShaderCache.prototype.getShaderProgram = function (options) { const context = this._context; const vertexShaderText = vertexShaderSource.createCombinedVertexShader( - this._context + context ); const fragmentShaderText = fragmentShaderSource.createCombinedFragmentShader( - this._context + context ); const shaderProgram = new ShaderProgram({