Bug 1488

Summary: Graph: Resolve Performance Regression in RegionRenderer's ShaderKey Utilization
Product: [JogAmp] Jogl Reporter: Sven Gothel <sgothel>
Component: graphAssignee: Sven Gothel <sgothel>
Status: RESOLVED FIXED    
Severity: critical CC: sgothel
Priority: P1    
Version: 2.6.0   
Hardware: All   
OS: all   
Type: DEFECT SCM Refs:
ffe4e670c9d35121934c6f3c95067d9c18aee386 6cf158cc59e901b49ab54681e363d23492421a9d 1dcfdf71c09c6d774ac47012c05e09da4a085d7b 95e2519a0fc37d19d05bc0f4cc14f593eaae7dd9 b711ae5239b8581a197d468b2804cfeb8c4d6c94
Workaround: ---
Bug Depends on:    
Bug Blocks: 805    

Description Sven Gothel 2024-01-22 02:36:55 CET 
RegionRenderer's ShaderKey

Commit 6363ae5fb6975a6f2e7c1093ce81f25b699e3e61 changed 
RegionRenderer.useShaderProgram()'s shader mapping using a new ShaderKey instance.

Such ShaderKey instance is created every time @ RegionRenderer.useShaderProgram()
to retrieve the ShaderProgram from the HashMap<ShaderKey, Shader Program>.
While this is most correct, creating the ShaderKey instance causes 
a lot of temp objects. 

ShaderKey also uses the optional colorTexSeq shader code for equality test
in case of hash-collisions.

Previous code simply ignored hash-collisions and used a 1:1 hashCode -> ShaderProgram mapping using our IntObjectHashMap. 
However, there was no test whether collision occur. 

+++

Solution would be either
1- Revert fully to the previous code just using an IntObjectHashMap,
   but throwing a RuntimeException in case of hashCode collisions.
   In case of a collisions, we would need to produce a better hashCode.
   This is possible, as the underlying data is fully internal .. etc.

2- Use the IntObjectHashMap as long there is no hashCode collision,
   then revert back to HashMap<ShaderKey, Shader Program>.

I favor variant (1), if it is possible to validate the hashCode.
Comment 1 Sven Gothel 2024-01-22 05:57:05 CET 
commit ffe4e670c9d35121934c6f3c95067d9c18aee386
implements solution 1, i.e. throws exception on shader hash-code collision.

Code supplies both variants, however, 
after already having used IntObjectHashMap w/o actual 
ShaderKey artifacts we have to think about whether fallback 
is even possible.
Otherwise .. in case we have no internal collision, this should be good enough.
Comment 2 Sven Gothel 2024-01-22 06:45:04 CET 
commit 6cf158cc59e901b49ab54681e363d23492421a9d (HEAD -> master)
    Bug 1488 - Graph RegionRenderer: Ensure shaderPrograms1 path is disabled using 'static final boolean useShaderPrograms0 = true'
Comment 3 Sven Gothel 2024-01-22 08:39:05 CET 
Closing for now - may need to be reopened in case 
commit ffe4e670c9d35121934c6f3c95067d9c18aee386
is not sufficient, see comment 1
Comment 4 Sven Gothel 2024-01-22 22:03:21 CET 
commit 1dcfdf71c09c6d774ac47012c05e09da4a085d7b
RegionRenderer: Use a more deterministic 64-bit shaderKey: 
[ 0-31] bit values and state, 
[32-63] colorTexSeqHash

This leaves only room for a key collision on the 32-bit colorTexSeqHash value
and hence should be save within our shader-code environment.

+        //  # |  s |
+        //  0 |  1 | isTwoPass
+        //  1 |  1 | pass1
+        //  2 |  5 | ShaderModeSelector1
+        //  7 |  1 | hasFrustumClipping
+        //  8 |  1 | hasColorChannel
+        //  9 |  1 | hasColorTexture
+        // 32 | 32 | colorTexSeqHash
+        long hash =             ( isTwoPass ? 1 : 0 );
+        hash = ( hash << 1 ) | ( pass1 ? 1 : 0 ) ;
+        hash = ( hash << 1 ) | sms.ordinal(); // incl. pass2Quality + sampleCount
+        hash = ( hash << 5 ) | ( hasFrustumClipping ? 1 : 0 );
+        hash = ( hash << 1 ) | ( hasColorChannel ? 1 : 0 );
+        hash = ( hash << 1 ) | ( hasColorTexture ? 1 : 0 );
+        hash = ( hash << 1 ) | ( ( colorTexSeqHash & 0xFFFFFFL ) << 32 );
Comment 5 Sven Gothel 2024-01-22 22:05:22 CET 
commit 95e2519a0fc37d19d05bc0f4cc14f593eaae7dd9
FFMPEGMediaPlayer: Fix getTextureFragmentShaderHashID(), i.e. use actual tc_w_1 = (float)getWidth() / (float)texWidth value as hardcoded within the shader
Comment 6 Sven Gothel 2024-01-25 09:22:19 CET 
(In reply to Sven Gothel from comment #4)
commit b711ae5239b8581a197d468b2804cfeb8c4d6c94
Bug 1488: Complete/Fix producing the 64-bit shaderKey: Use long values in bit-shift expressions and simplify it

commit 1dcfdf71c09c6d774ac47012c05e09da4a085d7b 
- still used the 'hash code' bit shift pattern, not necessary -> simplified
- the value as not ensured to be long, hence conversion occured

This caused Graph's MSAA not being picked up properly using the shaderKey.

-        long hash =             ( isTwoPass ? 1 : 0 );
-        hash = ( hash << 1 ) | ( pass1 ? 1 : 0 ) ;
-        hash = ( hash << 1 ) | sms.ordinal(); // incl. pass2Quality + sampleCount
-        hash = ( hash << 5 ) | ( hasFrustumClipping ? 1 : 0 );
-        hash = ( hash << 1 ) | ( hasColorChannel ? 1 : 0 );
-        hash = ( hash << 1 ) | ( hasColorTexture ? 1 : 0 );
-        hash = ( hash << 1 ) | ( ( colorTexSeqHash & 0xFFFFFFL ) << 32 );
+        long hash =  isTwoPass ? 1L : 0L;
+        hash |= ( pass1 ? 1L : 0L )               << 1;
+        hash |= (long)( sms.ordinal() & 0b11111 ) << 2; // incl. pass2Quality + sampleCount
+        hash |= ( hasFrustumClipping ? 1L : 0L )  << 7;
+        hash |= ( hasColorChannel ? 1L : 0L )     << 8;
+        hash |= ( hasColorTexture ? 1L : 0L )     << 9;
+        hash |= ( colorTexSeqHash & 0xFFFFFFL )   << 32;