Skip to content
This repository

Cache shader uniform lookup #358

Closed
wants to merge 9 commits into from

5 participants

Tim Mutton Laurent Gomila Mario Liebisch Michael Bradshaw Foaly
Tim Mutton

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

Laurent Gomila

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
Tim Mutton

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

Laurent Gomila

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.

Mario Liebisch

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).

Laurent Gomila

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.

Mario Liebisch

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.

Tim Mutton
Tim Mutton

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

Michael Bradshaw
mjbshaw commented

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).

Foaly
Foaly commented

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

Laurent Gomila
Owner

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.

Tim Mutton

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.

Foaly
Foaly commented

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
This page is out of date. Refresh to see the latest.

Showing 2 changed files with 114 additions and 28 deletions. Show diff stats Hide diff stats

  1. +27 3 include/SFML/Graphics/Shader.hpp
  2. +87 25 src/SFML/Graphics/Shader.cpp
30 include/SFML/Graphics/Shader.hpp
@@ -211,6 +211,27 @@ public :
211 211 bool loadFromStream(InputStream& vertexShaderStream, InputStream& fragmentShaderStream);
212 212
213 213 ////////////////////////////////////////////////////////////
  214 + /// \brief Change an integer parameter of the shader
  215 + ///
  216 + /// \a name is the name of the variable to change in the shader.
  217 + /// The corresponding parameter in the shader must be an integer
  218 + /// (intt GLSL type).
  219 + ///
  220 + /// Example:
  221 + /// \code
  222 + /// uniform int myparam; // this is the variable in the shader
  223 + /// \endcode
  224 + /// \code
  225 + /// shader.setParameter("myparam", 5);
  226 + /// \endcode
  227 + ///
  228 + /// \param name Name of the parameter in the shader
  229 + /// \param x Value to assign
  230 + ///
  231 + ////////////////////////////////////////////////////////////
  232 + void setParameter(const std::string& name, int x);
  233 +
  234 + ////////////////////////////////////////////////////////////
214 235 /// \brief Change a float parameter of the shader
215 236 ///
216 237 /// \a name is the name of the variable to change in the shader.
@@ -483,6 +504,7 @@ public :
483 504 static bool isAvailable();
484 505
485 506 private :
  507 + int getParamLocation(const std::string& name);
486 508
487 509 ////////////////////////////////////////////////////////////
488 510 /// \brief Compile the shader(s) and create the program
@@ -511,13 +533,15 @@ private :
511 533 // Types
512 534 ////////////////////////////////////////////////////////////
513 535 typedef std::map<int, const Texture*> TextureTable;
  536 + typedef std::map<std::string, int> ParameterCache;
514 537
515 538 ////////////////////////////////////////////////////////////
516 539 // Member data
517 540 ////////////////////////////////////////////////////////////
518   - unsigned int m_shaderProgram; ///< OpenGL identifier for the program
519   - int m_currentTexture; ///< Location of the current texture in the shader
520   - TextureTable m_textures; ///< Texture variables in the shader, mapped to their location
  541 + unsigned int m_shaderProgram; ///< OpenGL identifier for the program
  542 + int m_currentTexture; ///< Location of the current texture in the shader
  543 + TextureTable m_textures; ///< Texture variables in the shader, mapped to their location
  544 + ParameterCache m_params; ///< Uniforms in the shader, mapped to their location
521 545 };
522 546
523 547 } // namespace sf
112 src/SFML/Graphics/Shader.cpp
@@ -217,6 +217,32 @@ bool Shader::loadFromStream(InputStream& vertexShaderStream, InputStream& fragme
217 217
218 218
219 219 ////////////////////////////////////////////////////////////
  220 +void Shader::setParameter(const std::string& name, int x)
  221 +{
  222 + if (m_shaderProgram)
  223 + {
  224 + ensureGlContext();
  225 +
  226 + // Enable program
  227 + GLint program;
  228 + glGetIntegerv(GL_CURRENT_PROGRAM, &program);
  229 +
  230 + bool notActiveProgram = (static_cast<unsigned int>(program) != m_shaderProgram);
  231 + if(notActiveProgram) glCheck(glUseProgram(static_cast<GLuint>(m_shaderProgram)));
  232 +
  233 + // Get parameter location and assign it new values
  234 + GLint location = static_cast<GLint>(getParamLocation(name));
  235 + if (location != -1)
  236 + glCheck(glUniform1iARB(location, x));
  237 + else
  238 + err() << "Parameter \"" << name << "\" not found in shader" << std::endl;
  239 +
  240 + // Disable program
  241 + if(notActiveProgram) glCheck(glUseProgram(static_cast<GLuint>(program)));
  242 + }
  243 +}
  244 +
  245 +////////////////////////////////////////////////////////////
220 246 void Shader::setParameter(const std::string& name, float x)
221 247 {
222 248 if (m_shaderProgram)
@@ -224,18 +250,21 @@ void Shader::setParameter(const std::string& name, float x)
224 250 ensureGlContext();
225 251
226 252 // Enable program
227   - GLhandleARB program = glGetHandleARB(GL_PROGRAM_OBJECT_ARB);
228   - glCheck(glUseProgramObjectARB(m_shaderProgram));
  253 + GLint program;
  254 + glGetIntegerv(GL_CURRENT_PROGRAM, &program);
  255 +
  256 + bool notActiveProgram = (static_cast<unsigned int>(program) != m_shaderProgram);
  257 + if(notActiveProgram) glCheck(glUseProgram(static_cast<GLuint>(m_shaderProgram)));
229 258
230 259 // Get parameter location and assign it new values
231   - GLint location = glGetUniformLocationARB(m_shaderProgram, name.c_str());
  260 + GLint location = static_cast<GLint>(getParamLocation(name));
232 261 if (location != -1)
233 262 glCheck(glUniform1fARB(location, x));
234 263 else
235 264 err() << "Parameter \"" << name << "\" not found in shader" << std::endl;
236 265
237 266 // Disable program
238   - glCheck(glUseProgramObjectARB(program));
  267 + if(notActiveProgram) glCheck(glUseProgram(static_cast<GLuint>(program)));
239 268 }
240 269 }
241 270
@@ -248,18 +277,21 @@ void Shader::setParameter(const std::string& name, float x, float y)
248 277 ensureGlContext();
249 278
250 279 // Enable program
251   - GLhandleARB program = glGetHandleARB(GL_PROGRAM_OBJECT_ARB);
252   - glCheck(glUseProgramObjectARB(m_shaderProgram));
  280 + GLint program;
  281 + glGetIntegerv(GL_CURRENT_PROGRAM, &program);
  282 +
  283 + bool notActiveProgram = (static_cast<unsigned int>(program) != m_shaderProgram);
  284 + if(notActiveProgram) glCheck(glUseProgram(static_cast<GLuint>(m_shaderProgram)));
253 285
254 286 // Get parameter location and assign it new values
255   - GLint location = glGetUniformLocationARB(m_shaderProgram, name.c_str());
  287 + GLint location = static_cast<GLint>(getParamLocation(name));
256 288 if (location != -1)
257 289 glCheck(glUniform2fARB(location, x, y));
258 290 else
259 291 err() << "Parameter \"" << name << "\" not found in shader" << std::endl;
260 292
261 293 // Disable program
262   - glCheck(glUseProgramObjectARB(program));
  294 + if(notActiveProgram) glCheck(glUseProgram(static_cast<GLuint>(program)));
263 295 }
264 296 }
265 297
@@ -272,22 +304,24 @@ void Shader::setParameter(const std::string& name, float x, float y, float z)
272 304 ensureGlContext();
273 305
274 306 // Enable program
275   - GLhandleARB program = glGetHandleARB(GL_PROGRAM_OBJECT_ARB);
276   - glCheck(glUseProgramObjectARB(m_shaderProgram));
  307 + GLint program;
  308 + glGetIntegerv(GL_CURRENT_PROGRAM, &program);
  309 +
  310 + bool notActiveProgram = (static_cast<unsigned int>(program) != m_shaderProgram);
  311 + if(notActiveProgram) glCheck(glUseProgram(static_cast<GLuint>(m_shaderProgram)));
277 312
278 313 // Get parameter location and assign it new values
279   - GLint location = glGetUniformLocationARB(m_shaderProgram, name.c_str());
  314 + GLint location = static_cast<GLint>(getParamLocation(name));
280 315 if (location != -1)
281 316 glCheck(glUniform3fARB(location, x, y, z));
282 317 else
283 318 err() << "Parameter \"" << name << "\" not found in shader" << std::endl;
284 319
285 320 // Disable program
286   - glCheck(glUseProgramObjectARB(program));
  321 + if(notActiveProgram) glCheck(glUseProgram(static_cast<GLuint>(program)));
287 322 }
288 323 }
289 324
290   -
291 325 ////////////////////////////////////////////////////////////
292 326 void Shader::setParameter(const std::string& name, float x, float y, float z, float w)
293 327 {
@@ -296,18 +330,21 @@ void Shader::setParameter(const std::string& name, float x, float y, float z, fl
296 330 ensureGlContext();
297 331
298 332 // Enable program
299   - GLhandleARB program = glGetHandleARB(GL_PROGRAM_OBJECT_ARB);
300   - glCheck(glUseProgramObjectARB(m_shaderProgram));
  333 + GLint program;
  334 + glGetIntegerv(GL_CURRENT_PROGRAM, &program);
  335 +
  336 + bool notActiveProgram = (static_cast<unsigned int>(program) != m_shaderProgram);
  337 + if(notActiveProgram) glCheck(glUseProgram(static_cast<GLuint>(m_shaderProgram)));
301 338
302 339 // Get parameter location and assign it new values
303   - GLint location = glGetUniformLocationARB(m_shaderProgram, name.c_str());
  340 + GLint location = static_cast<GLint>(getParamLocation(name));
304 341 if (location != -1)
305 342 glCheck(glUniform4fARB(location, x, y, z, w));
306 343 else
307 344 err() << "Parameter \"" << name << "\" not found in shader" << std::endl;
308 345
309 346 // Disable program
310   - glCheck(glUseProgramObjectARB(program));
  347 + if(notActiveProgram) glCheck(glUseProgram(static_cast<GLuint>(program)));
311 348 }
312 349 }
313 350
@@ -341,18 +378,21 @@ void Shader::setParameter(const std::string& name, const sf::Transform& transfor
341 378 ensureGlContext();
342 379
343 380 // Enable program
344   - GLhandleARB program = glGetHandleARB(GL_PROGRAM_OBJECT_ARB);
345   - glCheck(glUseProgramObjectARB(m_shaderProgram));
  381 + GLint program;
  382 + glGetIntegerv(GL_CURRENT_PROGRAM, &program);
  383 +
  384 + bool notActiveProgram = (static_cast<unsigned int>(program) != m_shaderProgram);
  385 + if(notActiveProgram) glCheck(glUseProgram(static_cast<GLuint>(m_shaderProgram)));
346 386
347 387 // Get parameter location and assign it new values
348   - GLint location = glGetUniformLocationARB(m_shaderProgram, name.c_str());
  388 + GLint location = static_cast<GLint>(getParamLocation(name));
349 389 if (location != -1)
350 390 glCheck(glUniformMatrix4fvARB(location, 1, GL_FALSE, transform.getMatrix()));
351 391 else
352 392 err() << "Parameter \"" << name << "\" not found in shader" << std::endl;
353 393
354 394 // Disable program
355   - glCheck(glUseProgramObjectARB(program));
  395 + if(notActiveProgram) glCheck(glUseProgram(static_cast<GLuint>(program)));
356 396 }
357 397 }
358 398
@@ -365,7 +405,7 @@ void Shader::setParameter(const std::string& name, const Texture& texture)
365 405 ensureGlContext();
366 406
367 407 // Find the location of the variable in the shader
368   - int location = glGetUniformLocationARB(m_shaderProgram, name.c_str());
  408 + int location = static_cast<GLint>(getParamLocation(name));
369 409 if (location == -1)
370 410 {
371 411 err() << "Texture \"" << name << "\" not found in shader" << std::endl;
@@ -403,7 +443,7 @@ void Shader::setParameter(const std::string& name, CurrentTextureType)
403 443 ensureGlContext();
404 444
405 445 // Find the location of the variable in the shader
406   - m_currentTexture = glGetUniformLocationARB(m_shaderProgram, name.c_str());
  446 + m_currentTexture = static_cast<GLint>(getParamLocation(name));
407 447 if (m_currentTexture == -1)
408 448 err() << "Texture \"" << name << "\" not found in shader" << std::endl;
409 449 }
@@ -418,7 +458,7 @@ void Shader::bind(const Shader* shader)
418 458 if (shader && shader->m_shaderProgram)
419 459 {
420 460 // Enable the program
421   - glCheck(glUseProgramObjectARB(shader->m_shaderProgram));
  461 + glCheck(glUseProgram(static_cast<GLuint>(shader->m_shaderProgram)));
422 462
423 463 // Bind the textures
424 464 shader->bindTextures();
@@ -430,7 +470,7 @@ void Shader::bind(const Shader* shader)
430 470 else
431 471 {
432 472 // Bind no shader
433   - glCheck(glUseProgramObjectARB(0));
  473 + glCheck(glUseProgram(0));
434 474 }
435 475 }
436 476
@@ -467,6 +507,8 @@ bool Shader::compile(const char* vertexShaderCode, const char* fragmentShaderCod
467 507 if (m_shaderProgram)
468 508 glCheck(glDeleteObjectARB(m_shaderProgram));
469 509
  510 + m_params.clear();
  511 +
470 512 // Create the program
471 513 m_shaderProgram = glCreateProgramObjectARB();
472 514
@@ -564,4 +606,24 @@ void Shader::bindTextures() const
564 606 glCheck(glActiveTextureARB(GL_TEXTURE0_ARB));
565 607 }
566 608
  609 +
  610 +////////////////////////////////////////////////////////////
  611 +int Shader::getParamLocation(const std::string& name)
  612 +{
  613 + ParameterCache::const_iterator it = m_params.lower_bound(name);
  614 +
  615 + if((it != m_params.end() && !(m_params.key_compare()(name, it->first)))
  616 + {
  617 + return it->second;
  618 + }
  619 + else
  620 + {
  621 + int location = static_cast<int>(glGetUniformLocationARB(m_shaderProgram, name.c_str()));
  622 + if (location != -1)
  623 + m_params.insert(it, ParameterCache::value_type(name, location));
  624 +
  625 + return location;
  626 + }
  627 +}
  628 +
567 629 } // namespace sf

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.