From 2a3a1c7bb36410c6037f1d8da46fa68733efbe42 Mon Sep 17 00:00:00 2001 From: Aleksandar Stojiljkovic Date: Mon, 20 May 2019 16:17:19 +0300 Subject: [core] Remove re-linking programs approach (#14482) Remove re-linking programs as redundant. It costs (cheaper to link once than twice) and (subjective) is not that common GL API usage pattern, although perfectly legal and permitted. Initial idea, behind the removed code, was to enable work on optimization that would reduce number of attrib setup calls in case when VAO is not available (as described in #9433). As such optimization is not implemented, and it is arguable if it makes sense to do it now, we can remove re-linking. Related to closed PRs #9433 and PR #11583. I have [measured the time spent just on relinking](https://gist.github.com/astojilj/29bd5a5c5dc0b2d9f29ecb660da07fbf) using release build on iPhone SE (A9, same as iPhone 6S): - 1st run after reboot or installation Total 37.14ms, average per program:1.86ms - reopening Total: 2.47ms, average per program: 0.12ms This time we save using the patch here. --- src/mbgl/gl/attribute.cpp | 38 +++++--------------------------------- src/mbgl/gl/attribute.hpp | 47 +++++++++++++++++++---------------------------- src/mbgl/gl/context.cpp | 8 +++++++- src/mbgl/gl/context.hpp | 2 +- src/mbgl/gl/program.hpp | 11 +++-------- 5 files changed, 35 insertions(+), 71 deletions(-) (limited to 'src') diff --git a/src/mbgl/gl/attribute.cpp b/src/mbgl/gl/attribute.cpp index 4983a8c204..d5240b5b7a 100644 --- a/src/mbgl/gl/attribute.cpp +++ b/src/mbgl/gl/attribute.cpp @@ -1,47 +1,19 @@ #include #include -#include namespace mbgl { namespace gl { using namespace platform; -void bindAttributeLocation(Context& context, ProgramID id, AttributeLocation location, const char* name) { - // We're using sequentially numberered attribute locations starting with 0. Therefore we can use - // the location as a proxy for the number of attributes. - if (location >= context.maximumVertexBindingCount) { - // Don't bind the location on this hardware since it exceeds the limit (otherwise we'd get - // an OpenGL error). This means we'll see rendering errors, and possibly slow rendering due - // to unbound attributes. +optional queryLocation(ProgramID id, const char* name) { + GLint attributeLocation = MBGL_CHECK_ERROR(glGetAttribLocation(id, name)); + if (attributeLocation != -1) { + return attributeLocation; } else { - MBGL_CHECK_ERROR(glBindAttribLocation(id, location, name)); + return {}; } } -std::set getActiveAttributes(ProgramID id) { - std::set activeAttributes; - - GLint attributeCount; - MBGL_CHECK_ERROR(glGetProgramiv(id, GL_ACTIVE_ATTRIBUTES, &attributeCount)); - - GLint maxAttributeLength; - MBGL_CHECK_ERROR(glGetProgramiv(id, GL_ACTIVE_ATTRIBUTE_MAX_LENGTH, &maxAttributeLength)); - - std::string attributeName; - attributeName.resize(maxAttributeLength); - - GLsizei actualLength; - GLint size; - GLenum type; - - for (int32_t i = 0; i < attributeCount; i++) { - MBGL_CHECK_ERROR(glGetActiveAttrib(id, i, maxAttributeLength, &actualLength, &size, &type, &attributeName[0])); - activeAttributes.emplace(std::string(attributeName, 0, actualLength)); - } - - return activeAttributes; -} - } // namespace gl } // namespace mbgl diff --git a/src/mbgl/gl/attribute.hpp b/src/mbgl/gl/attribute.hpp index c7f9ba3fd4..c2e362da63 100644 --- a/src/mbgl/gl/attribute.hpp +++ b/src/mbgl/gl/attribute.hpp @@ -2,18 +2,12 @@ #include #include -#include +#include #include -#include #include -#include #include -#include -#include #include -#include -#include namespace mbgl { namespace gl { @@ -21,9 +15,7 @@ namespace gl { using AttributeBindingArray = std::vector>; using NamedAttributeLocations = std::vector>; -class Context; -void bindAttributeLocation(Context&, ProgramID, AttributeLocation, const char * name); -std::set getActiveAttributes(ProgramID); +optional queryLocation(ProgramID id, const char* name); template class AttributeLocations; @@ -37,24 +29,7 @@ private: Locations locations; public: - AttributeLocations(Context& context, const ProgramID& id) - : locations([&] { - std::set activeAttributes = getActiveAttributes(id); - - AttributeLocation location = 0; - auto maybeBindLocation = [&](const char* name) -> optional { - if (activeAttributes.count(name)) { - bindAttributeLocation(context, id, location, name); - return location++; - } else { - return {}; - } - }; - - return Locations{ maybeBindLocation( - concat_literals<&string_literal<'a', '_'>::value, &As::name>::value())... }; - }()) { - } + AttributeLocations() = default; template AttributeLocations(const BinaryProgram& program) @@ -62,6 +37,22 @@ public: concat_literals<&string_literal<'a', '_'>::value, &As::name>::value())... } { } + void queryLocations(const ProgramID& id) { + locations = Locations{ + queryLocation(id, concat_literals<&string_literal<'a', '_'>::value, &As::name>::value())... }; + using TypeOfFirst = typename std::tuple_element<0, std::tuple>::type; + auto first = locations.template get(); + assert(first && first.value() == 0); + } + + static constexpr const char* getFirstAttribName() { + // Static assert that attribute list starts with position: we bind it on location 0. + using TypeOfFirst = typename std::tuple_element<0, std::tuple>::type; + static_assert(std::is_same::value || std::is_same::value || + std::is_same::value, "Program must start with position related attribute."); + return concat_literals<&string_literal<'a', '_'>::value, TypeOfFirst::name>::value(); + } + NamedAttributeLocations getNamedLocations() const { NamedAttributeLocations result; diff --git a/src/mbgl/gl/context.cpp b/src/mbgl/gl/context.cpp index f1288dc7ed..da72bad41d 100644 --- a/src/mbgl/gl/context.cpp +++ b/src/mbgl/gl/context.cpp @@ -169,12 +169,18 @@ UniqueShader Context::createShader(ShaderType type, const std::initializer_list< throw std::runtime_error("shader failed to compile"); } -UniqueProgram Context::createProgram(ShaderID vertexShader, ShaderID fragmentShader) { +UniqueProgram Context::createProgram(ShaderID vertexShader, ShaderID fragmentShader, const char* location0AttribName) { UniqueProgram result { MBGL_CHECK_ERROR(glCreateProgram()), { this } }; MBGL_CHECK_ERROR(glAttachShader(result, vertexShader)); MBGL_CHECK_ERROR(glAttachShader(result, fragmentShader)); + // It is important to have attribute at position 0 enabled: conveniently, + // position attribute is always first and always enabled. The integrity of + // this assumption is verified in AttributeLocations::queryLocations and + // AttributeLocations::getFirstAttribName. + MBGL_CHECK_ERROR(glBindAttribLocation(result, 0, location0AttribName)); + linkProgram(result); return result; diff --git a/src/mbgl/gl/context.hpp b/src/mbgl/gl/context.hpp index 41e5b5f22d..cf01733adf 100644 --- a/src/mbgl/gl/context.hpp +++ b/src/mbgl/gl/context.hpp @@ -50,7 +50,7 @@ public: void enableDebugging(); UniqueShader createShader(ShaderType type, const std::initializer_list& sources); - UniqueProgram createProgram(ShaderID vertexShader, ShaderID fragmentShader); + UniqueProgram createProgram(ShaderID vertexShader, ShaderID fragmentShader, const char* location0AttribName); UniqueProgram createProgram(BinaryProgramFormat binaryFormat, const std::string& binaryProgram); void verifyProgramLinkage(ProgramID); void linkProgram(ProgramID); diff --git a/src/mbgl/gl/program.hpp b/src/mbgl/gl/program.hpp index 3757c442de..8540385e97 100644 --- a/src/mbgl/gl/program.hpp +++ b/src/mbgl/gl/program.hpp @@ -49,15 +49,10 @@ public: const std::initializer_list& fragmentSource) : program(context.createProgram( context.createShader(ShaderType::Vertex, vertexSource), - context.createShader(ShaderType::Fragment, fragmentSource))), - attributeLocations(context, program) { - // Re-link program after manually binding only active attributes in Attributes::queryLocations - context.linkProgram(program); - - // We have to re-initialize the uniforms state from the bindings as the uniform locations - // get shifted on some implementations + context.createShader(ShaderType::Fragment, fragmentSource), + attributeLocations.getFirstAttribName())) { + attributeLocations.queryLocations(program); uniformStates.queryLocations(program); - // Texture units are specified via uniforms as well, so we need query their locations textureStates.queryLocations(program); } -- cgit v1.2.1