Cache uniform locations in ffglex::FFGLShader - #107
Open
vjandrea wants to merge 1 commit into
Open
Conversation
FindUniform called glGetUniformLocation unconditionally on every call, even though a uniform's location is fixed for the lifetime of a linked program. Adds a name->location cache, populated lazily in FindUniform and cleared whenever the underlying program changes (FreeGLResources, and defensively at the top of LinkProgram). Fixes resolume#80.
Author
|
@MennoVink This is the same request as #83, closed without merging even after the |
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.
FFGLShader::FindUniformcalledglGetUniformLocationunconditionally on every call. EverySet(...)overload routes through it, so a plugin setting a uniform every frame (e.g.
Particles, which callsFindUniformby name around 19 times per frame across two shaders) paid a string-keyed driver round-trip peruniform per frame, per instance, for no benefit: a uniform's location is fixed for the lifetime of a linked
program.
Adds a
name → locationcache toFFGLShader, populated lazily inFindUniformand invalidated (cleared)whenever the underlying program changes: in
FreeGLResources(), and defensively at the top ofLinkProgram(). No public API change.Fixes #80.
Notes for reviewers:
uniformLocationsnever evicts entries. Thisis OK in practice because it's bounded by the number of distinct uniform names a given shader declares
(single digits up to
Particles' ~19, the largest user in this tree) not by frame count or anythingthat grows over the plugin's lifetime. An eviction policy would add complexity for a cache that already
self-limits at the shader's actual uniform count.
LinkProgram()is currently dead code. I traced every caller ofCompile()/FreeGLResources()in this repo (Add,AddSubtract,Gradients,CustomThumbnail,Particles'GLResources,ffglquickstart::Plugin) and all of them follow the sameInitGL → Compile()thenDeInitGL → FreeGLResources()lifecycle, once each, per instance, and none of themcall
Compile()a second time on an already-linked shader without freeing it first. So today, clearing inFreeGLResources()alone would be enough to keep the cache correct. I kept the extra clear inLinkProgram()anyway as cheap insurance: nothing inFFGLShader's public API actually prevents a futurecaller from calling
Compile()twice without an interveningFreeGLResources(), and if that everhappened, the old cache entries would silently point at locations from the previous (now-replaced)
programIDwithout this clear. Flagging so it doesn't read as accidental defensive-programming clutter becauseit's addressing a real (if currently unexercised) gap in the class's invariants. Please don't hesitate to flag it for removal anyway, it if you think that it's just premature optimization.