Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Cache shader uniform lookup #358

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
6 participants

Updated shader to store uniform locations in a map, which should increase performance for uniform lookups

Owner

LaurentGomila commented Feb 20, 2013

Have you read #316 first?

Like I said, until someone complains about it, I won't change that.

Your modification can be improved:

  • the cache should be cleared when the shader is reloaded
  • you should replace GLint with int, in order to avoid exposing GL headers in the public SFML headers

I've implemented the suggested changes. My understanding is that 316 was rejected because it changed the public interface, whereas my changes only alter the internals

Owner

LaurentGomila commented Feb 20, 2013

My understanding is that 316 was rejected because it changed the public interface, whereas my changes only alter the internals

It's true. However I don't feel like this modification is needed -- maybe in the future.

Member

MarioLiebisch commented Feb 20, 2013

Not sure how the compiler optimized result will look like, but right now you're performing two lookups in case the element is found. Why don't you use the result of std::map::find() for your return value? Also I'd consider using a hash map (custom hash class as key?), because otherwise the lookup can still get quite expensive (esp. in older versions of of MSVC).

Owner

LaurentGomila commented Feb 20, 2013

Also I'd consider using a hash map

Hash maps are not standard in C++03. And I wouldn't consider a log(n) lookup expensive on a set of less than 100 elements.

Member

MarioLiebisch commented Feb 20, 2013

I'm talking about a hash class working as the key, comparing a long int or something similar (or just a sf::String member returning an integer hash?). Not talking about MSVC's stdext::hash_map. But yeah, real question is, will the difference be noticeable.

I've updated the code to return the iterator value instead of looking it up
again. An unordered_map would be better suited to this if/when sfml moves
over to C++11. As for hashing the string first, if the interface wasnt
changed (so uniforms were still looked up by name) the penalty of hashing
the value would offset the benefit of using a hashed value

On Thu, Feb 21, 2013 at 6:47 AM, Mario Liebisch notifications@github.comwrote:

I'm talking about a hash class working as the key, comparing a long intor something similar (or just a
sf::String member returning an integer hash?). Not talking about MSVC's
stdext::hash_map.


Reply to this email directly or view it on GitHubhttps://github.com/LaurentGomila/SFML/pull/358#issuecomment-13852289.

Ive made a couple more updates to the shader. The first is that I've added the ability to set an int through the shader. Another update I've made is to use glGet to retrieve the current program, and use that to check if the shader setting the parameter is already the active shader, in which case it wont call glUseProgram twice unnecessarily

mjbshaw commented May 16, 2013

If you wanted to be pedantic about the algorithmic optimization of this, Shader::getParamLocation() could be changed to use lower_bound instead of find and then use the returned iterator to insert into the map if it doesn't exist. See this SO question.

Just a thought (and probably overkill).

Contributor

Foaly commented May 16, 2013

I think it would be a great addition to SFML if the speed of te uniform lookup would be improved.

Owner

LaurentGomila commented May 16, 2013

I think it would be a great addition to SFML if the speed of te uniform lookup would be improved.

Really? You have speed problems with that? I'm curious to see your benchmark results.

I've updated the code to include mjbshaw's suggestion. I'm aware that the speedup provided by my solution doesn't make a huge difference, however it's still a performance gain and the work is already done, may as well take advantage of it.

Contributor

Foaly commented Jun 9, 2013

I don't have any benchmarks. But I did a project a while back, that made use of shaders quiet extensiv. For that I implemented something similar (this look cleaner though). I did some measurements back then, but I don't have anything that is reproducable. But I agree with timmutton. I don't see anything that speaks against an additon that brings a performance improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment