Avoid redundant uniform uploads #2090

juj · 2020-03-30T16:54:29Z

Manage a cache of current uniform state to avoid reuploading state that is already on the GPU side.

In a WebGL build, this was observed to reduce the number of GL calls from ~21k/frame to about ~7.5k/frame.

src/renderer_gl.cpp

juj · 2020-04-06T16:36:09Z

If using std::unordered_map instead of the custom hash table implementation, frame times get +7.9% slower. (code size increases by +0.39% vs TinyHashMap)

If using tinystl::unordered_map instead of the custom hash table implementation, frame times are +3.3% slower. (code size increases by +0.15% vs TinyHashMap)

I could go with tinystl::unordered_map instead of TinyHashMap, though that does have a measurable perf drop. :/

bkaradzic · 2020-04-07T02:16:10Z

You should use tinystl::unordered_map, also feel free to apply TinyHashMap optimizations to it since it's used elsewhere in bgfx, so those optimizations would add up.

…tures

juj · 2020-04-07T09:44:47Z

Unfortunately it is not possible to optimize tinystl::unordered_map with same means - TinyHashMap operated on different semantics (combined insert+find, and no need for individual element removal support) that made it better for this purpose.

But dropped TinyHashMap in favor of tinystl::unordered_map to conform to BGFX data structures. How does this look now?

bkaradzic · 2020-04-07T16:29:57Z

src/renderer_gl.cpp

-			delete[] table;
-		}
+		// Uniform state cache for various types.
+		static tinystl::unordered_map<uint64_t, T> uniformCacheMap;


static tinystl::unordered_map<uint64_t, T> uniformCacheMap;

Do not make it static, make it part of GL renderer context so that it can be deallocated properly.

Also use stl::unordered_map (it's just name toggle between std/tinystl).

Also make some #if BGFX_GL_CONFIG_UNIFORM_CACHE so that this code path can be enabled/disabled.

bkaradzic · 2020-04-07T16:34:39Z

src/renderer_gl.cpp

-			delete[] table;
-		}
+		// Uniform state cache for various types.
+		static tinystl::unordered_map<uint64_t, T> uniformCacheMap;


Also make some #if BGFX_GL_CONFIG_UNIFORM_CACHE so that this code path can be enabled/disabled.

bkaradzic · 2020-04-08T05:47:06Z

src/renderer_gl.cpp

-	static GLuint currentProgram = 0;
+	// Keep a global access to the currently active uniform state cache so that
+	// individual GL functions can stay at global scope.
+	static UniformStateCache *s_currentUniformStateCache = 0;


See: s_renderGL at line 4313. You can make these functions part of RendererContextGL or you can forward declare them at this point, and implement them below RendererContextGL.

bkaradzic · 2020-04-08T06:12:50Z

I'm getting compiler error because those templated functions are templated on return value.

juj · 2020-04-08T06:16:55Z

Would have been great to wait for the CI to go through to see that it works. What are the errors? Which compiler? With LLVM 6.0 and LLVM 10.0 it does build ok. Templating on return value by itself is not a problem, that will just require one to explicitly type specify the <T> when calling.

bkaradzic · 2020-04-08T06:20:12Z

../../linux64_gcc/bin/libbgfxDebug.a(glcontext_glx.o): In function `bx::ignoreC4127(bool)':
<...>/_github/bgfx/.build/projects/gmake-linux/../../../src/renderer_gl.h:1294: multiple definition of `tinystl::unordered_map<unsigned long, int, bgfx::TinyStlAllocator>& bgfx::gl::UniformStateCache::getUniformCache<int>()'
../../linux64_gcc/bin/libbgfxDebug.a(renderer_gl.o):<...>/_github/bgfx/.build/projects/gmake-linux/../../../src/renderer_gl.h:1294: first defined here
../../linux64_gcc/bin/libbgfxDebug.a(glcontext_glx.o): In function `tinystl::unordered_map<unsigned long, bgfx::gl::UniformStateCache::f4, bgfx::TinyStlAllocator>& bgfx::gl::UniformStateCache::getUniformCache<bgfx::gl::UniformStateCache::f4>()':
<...>/_github/bgfx/.build/projects/gmake-linux/../../../src/renderer_gl.h:1295: multiple definition of `tinystl::unordered_map<unsigned long, bgfx::gl::UniformStateCache::f4, bgfx::TinyStlAllocator>& bgfx::gl::UniformStateCache::getUniformCache<bgfx::gl::UniformStateCache::f4>()'
../../linux64_gcc/bin/libbgfxDebug.a(renderer_gl.o):<...>/_github/bgfx/.build/projects/gmake-linux/../../../src/renderer_gl.h:1295: first defined here
../../linux64_gcc/bin/libbgfxDebug.a(glcontext_glx.o): In function `tinystl::unordered_map<unsigned long, bgfx::gl::UniformStateCache::f3x3, bgfx::TinyStlAllocator>& bgfx::gl::UniformStateCache::getUniformCache<bgfx::gl::UniformStateCache::f3x3>()':
<...>/_github/bgfx/.build/projects/gmake-linux/../../../src/renderer_gl.h:1296: multiple definition of `tinystl::unordered_map<unsigned long, bgfx::gl::UniformStateCache::f3x3, bgfx::TinyStlAllocator>& bgfx::gl::UniformStateCache::getUniformCache<bgfx::gl::UniformStateCache::f3x3>()'
../../linux64_gcc/bin/libbgfxDebug.a(renderer_gl.o):<...>/_github/bgfx/.build/projects/gmake-linux/../../../src/renderer_gl.h:1296: first defined here
../../linux64_gcc/bin/libbgfxDebug.a(glcontext_glx.o): In function `tinystl::unordered_map<unsigned long, bgfx::gl::UniformStateCache::f4x4, bgfx::TinyStlAllocator>& bgfx::gl::UniformStateCache::getUniformCache<bgfx::gl::UniformStateCache::f4x4>()':
<...>/_github/bgfx/.build/projects/gmake-linux/../../../src/renderer_gl.h:1297: multiple definition of `tinystl::unordered_map<unsigned long, bgfx::gl::UniformStateCache::f4x4, bgfx::TinyStlAllocator>& bgfx::gl::UniformStateCache::getUniformCache<bgfx::gl::UniformStateCache::f4x4>()'
../../linux64_gcc/bin/libbgfxDebug.a(renderer_gl.o):<...>/_github/bgfx/.build/projects/gmake-linux/../../../src/renderer_gl.h:1297: first defined here
collect2: error: ld returned 1 exit status

bkaradzic · 2020-04-08T06:20:31Z

Would have been great to wait for the CI to go through to see that it works.

I assumed you compiled it... :)

gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0

This reverts commit d9d9865.

bkaradzic · 2020-04-08T06:24:53Z

Reverted it c023ac4...

Just revert it back, fix template issue, and resubmit PR, the rest looks good.

juj · 2020-04-08T06:34:56Z

Ok the issue is that GCC does not treat the template specializations inline. Adding inline to the four lines is the fix, but I am unsure if GCC wants to see the template declaration in class scope be inline as well, or if it will forbid it since it is not a definition. I do not have access to GCC, so I will remove the template approach altogether since I cannot test against it.

bkaradzic · 2020-04-08T06:44:57Z

Actually wait. I can add inline.

This reverts commit c023ac4.

bkaradzic · 2020-04-08T06:52:20Z

All good.

juj · 2020-04-08T08:19:57Z

Perfect, thanks!

* Avoid redundant uniform uploads * Fix placement of GL_CHECK()s and import of glUniform4f. * Fix typo * Migrate GL uniform cache to use tinystl to conform to BGFX data structures * Address review * Address review

This reverts commit d9d9865.

This reverts commit c023ac4.

bkaradzic self-requested a review March 30, 2020 21:56

bkaradzic requested changes Mar 30, 2020

View reviewed changes

src/renderer_gl.cpp Outdated Show resolved Hide resolved

src/renderer_gl.cpp Outdated Show resolved Hide resolved

juj force-pushed the optimize_redundant_uniform_uploads branch from 9682795 to d85b969 Compare April 6, 2020 10:44

juj added 3 commits April 6, 2020 19:43

Avoid redundant uniform uploads

0a39e24

Fix placement of GL_CHECK()s and import of glUniform4f.

c7d2d21

Fix typo

7f9ebc5

juj force-pushed the optimize_redundant_uniform_uploads branch from d85b969 to 7f9ebc5 Compare April 6, 2020 16:44

Migrate GL uniform cache to use tinystl to conform to BGFX data struc…

f3b94ee

…tures

bkaradzic requested changes Apr 7, 2020

View reviewed changes

Address review

3fb5cbb

bkaradzic requested changes Apr 8, 2020

View reviewed changes

Address review

9bfcae4

bkaradzic merged commit d9d9865 into bkaradzic:master Apr 8, 2020

bkaradzic added a commit that referenced this pull request Apr 8, 2020

Revert "Avoid redundant uniform uploads (#2090)"

c023ac4

This reverts commit d9d9865.

bkaradzic added a commit that referenced this pull request Apr 8, 2020

Revert "Revert "Avoid redundant uniform uploads (#2090)""

1361ccf

This reverts commit c023ac4.

rozgo pushed a commit to VertexStudio/bgfx that referenced this pull request Aug 14, 2020

Revert "Avoid redundant uniform uploads (bkaradzic#2090)"

cbaff71

This reverts commit d9d9865.

rozgo pushed a commit to VertexStudio/bgfx that referenced this pull request Aug 14, 2020

Revert "Revert "Avoid redundant uniform uploads (bkaradzic#2090)""

bc966a2

This reverts commit c023ac4.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid redundant uniform uploads #2090

Avoid redundant uniform uploads #2090

juj commented Mar 30, 2020

juj commented Apr 6, 2020

bkaradzic commented Apr 7, 2020

juj commented Apr 7, 2020

bkaradzic Apr 7, 2020 •

edited

Loading

bkaradzic Apr 7, 2020

bkaradzic Apr 7, 2020

bkaradzic Apr 8, 2020

bkaradzic commented Apr 8, 2020

juj commented Apr 8, 2020

bkaradzic commented Apr 8, 2020 •

edited

Loading

bkaradzic commented Apr 8, 2020 •

edited

Loading

bkaradzic commented Apr 8, 2020

juj commented Apr 8, 2020

bkaradzic commented Apr 8, 2020

bkaradzic commented Apr 8, 2020

juj commented Apr 8, 2020

Avoid redundant uniform uploads #2090

Avoid redundant uniform uploads #2090

Conversation

juj commented Mar 30, 2020

juj commented Apr 6, 2020

bkaradzic commented Apr 7, 2020

juj commented Apr 7, 2020

bkaradzic Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

bkaradzic Apr 7, 2020

Choose a reason for hiding this comment

bkaradzic Apr 7, 2020

Choose a reason for hiding this comment

bkaradzic Apr 8, 2020

Choose a reason for hiding this comment

bkaradzic commented Apr 8, 2020

juj commented Apr 8, 2020

bkaradzic commented Apr 8, 2020 • edited Loading

bkaradzic commented Apr 8, 2020 • edited Loading

bkaradzic commented Apr 8, 2020

juj commented Apr 8, 2020

bkaradzic commented Apr 8, 2020

bkaradzic commented Apr 8, 2020

juj commented Apr 8, 2020

bkaradzic Apr 7, 2020 •

edited

Loading

bkaradzic commented Apr 8, 2020 •

edited

Loading

bkaradzic commented Apr 8, 2020 •

edited

Loading