Skip to content

Commit

Permalink
GL: port label() / setLabel() away from std::string.
Browse files Browse the repository at this point in the history
All the tests were updated to explicitly check that non-null-terminated
strings get handled properly (the GL label APIs have an  explicit size,
so it *should*, but just in case). Also, because various subclasses
override the setter to return correct type for method chaining and the
override has to be deinlined to avoid relying on a StringView include,
the tests are now explicitly done for each leaf class, instead of the
subclass

The <string> being removed from the base class for all GL objects may
affect downstream projects which relied on it being included. In case of
Magnum, the breakages were already fixed in the previous commit.

Compile time improvement for the MagnumGL library alone is 0.2 second or
4% (6.1 seconds before, 5.9 after). Not bad, given that there's three
more files to compile and strings are still heavily used in other GL
debug output APIs and all shader stuff. For a build of just the GL
library and all tests, it goes down from 28.9 seconds to 28.1. Most
tests also still rely quite heavily on std::stringstream for debug
output testing, so the numbers still could go further.
  • Loading branch information
mosra committed Feb 19, 2022
1 parent c47393d commit bc88442
Show file tree
Hide file tree
Showing 68 changed files with 1,046 additions and 385 deletions.
35 changes: 21 additions & 14 deletions doc/changelog.dox
Original file line number Diff line number Diff line change
Expand Up @@ -892,20 +892,27 @@ See also:
@ref Corrade::Containers::ArrayView are now removed. This should have a
significant positive effect on compile times of code using the @ref GL,
@ref Audio, @ref Trade and @ref Text libraries
- As part of the ongoing STL header dependency cleanup,
@ref GL::Context::vendorString(),
@relativeref{GL::Context,rendererString()},
@relativeref{GL::Context,versionString()},
@relativeref{GL::Context,shadingLanguageVersionString()},
@relativeref{GL::Context,shadingLanguageVersionStrings()} and
@relativeref{GL::Context,extensionStrings()} now return
@relativeref{Corrade,Containers::StringView} or a
@relativeref{Corrade,Containers::Array} /
@relativeref{Corrade,Containers::ArrayView} of them, instead of a
@ref std::string and a @ref std::vector. For at least some backwards
compatibility the @ref Corrade/Containers/StringStl.h header is included to
provide implicit conversions to a @ref std::string, but in most cases
you'll be forced to change the code that uses those APIs.
- As part of the ongoing STL header dependency cleanup, the following
breaking changes related to @ref std::string and a @ref std::vector are
done:
- @ref GL::Context::vendorString(),
@relativeref{GL::Context,rendererString()},
@relativeref{GL::Context,versionString()},
@relativeref{GL::Context,shadingLanguageVersionString()},
@relativeref{GL::Context,shadingLanguageVersionStrings()} and
@relativeref{GL::Context,extensionStrings()} now return
@relativeref{Corrade,Containers::StringView} or a
@relativeref{Corrade,Containers::Array} /
@relativeref{Corrade,Containers::ArrayView} of them, instead of a
@ref std::string and a @ref std::vector.
- All @ref GL::Buffer::label() "label()" and
@ref GL::Buffer::setLabel() "setLabel()" APIs now work with a
@relativeref{Corrade,Containers::StringView} /
@relativeref{Corrade,Containers::String} instead of a @ref std::string
To handle most backwards compatibility, @ref Corrade/Containers/StringStl.h
is included in affected headers for implicit conversions from/to a
@ref std::string, but in some cases you may be forced to change the code
that uses those APIs.
- @ref GL::TextureFormat::SR8 and @ref GL::TextureFormat::SRG8 were present
on ES2 builds by mistake --- the @gl_extension{EXT,texture_sRGB_R8} and
@gl_extension{EXT,texture_sRGB_RG8} extensions require OpenGL ES 3.0 at
Expand Down
1 change: 1 addition & 0 deletions src/Magnum/GL/AbstractFramebuffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
* @brief Class @ref Magnum::GL::AbstractFramebuffer, enum @ref Magnum::GL::FramebufferClear, @ref Magnum::GL::FramebufferBlit, @ref Magnum::GL::FramebufferBlitFilter, @ref Magnum::GL::FramebufferTarget, enum set @ref Magnum::GL::FramebufferClearMask
*/

#include <utility> /* std::swap() */
#include <Corrade/Containers/EnumSet.h>

#include "Magnum/GL/GL.h"
Expand Down
59 changes: 25 additions & 34 deletions src/Magnum/GL/AbstractObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@

#include <Corrade/Utility/Assert.h>
#include <Corrade/Containers/ArrayView.h>
#ifndef MAGNUM_TARGET_WEBGL
#include <Corrade/Containers/String.h>
#endif

#include "Magnum/GL/Context.h"
#include "Magnum/GL/Extensions.h"
Expand Down Expand Up @@ -123,77 +126,65 @@ Int AbstractObject::maxLabelLength() {
return value;
}

void AbstractObject::labelImplementationNoOp(GLenum, GLuint, Containers::ArrayView<const char>) {}
void AbstractObject::labelImplementationNoOp(GLenum, GLuint, Containers::StringView) {}

#ifndef MAGNUM_TARGET_GLES2
void AbstractObject::labelImplementationKhrDesktopES32(const GLenum identifier, const GLuint name, const Containers::ArrayView<const char> label) {
glObjectLabel(identifier, name, label.size(), label);
void AbstractObject::labelImplementationKhrDesktopES32(const GLenum identifier, const GLuint name, const Containers::StringView label) {
glObjectLabel(identifier, name, label.size(), label.data());
}
#endif

#ifdef MAGNUM_TARGET_GLES
void AbstractObject::labelImplementationKhrES(const GLenum identifier, const GLuint name, const Containers::ArrayView<const char> label) {
glObjectLabelKHR(identifier, name, label.size(), label);
void AbstractObject::labelImplementationKhrES(const GLenum identifier, const GLuint name, const Containers::StringView label) {
glObjectLabelKHR(identifier, name, label.size(), label.data());
}
#endif

void AbstractObject::labelImplementationExt(const GLenum identifier, const GLuint name, const Containers::ArrayView<const char> label) {
void AbstractObject::labelImplementationExt(const GLenum identifier, const GLuint name, const Containers::StringView label) {
const GLenum type = extTypeFromKhrIdentifier(identifier);
glLabelObjectEXT(type, name, label.size(), label);
glLabelObjectEXT(type, name, label.size(), label.data());
}

std::string AbstractObject::getLabelImplementationNoOp(GLenum, GLuint) { return {}; }
Containers::String AbstractObject::getLabelImplementationNoOp(GLenum, GLuint) { return {}; }

#ifndef MAGNUM_TARGET_GLES2
std::string AbstractObject::getLabelImplementationKhrDesktopES32(const GLenum identifier, const GLuint name) {
Containers::String AbstractObject::getLabelImplementationKhrDesktopES32(const GLenum identifier, const GLuint name) {
/* Get label size (w/o null terminator). Specifying 0 as size is not
allowed, thus we pass the maximum instead. */
GLsizei size = 0;
glGetObjectLabel(identifier, name, maxLabelLength(), &size, nullptr);

/* Make place also for the null terminator */
std::string label;
label.resize(size+1);
glGetObjectLabel(identifier, name, size+1, nullptr, &label[0]);

/* Pop null terminator and return the string */
label.resize(size);
/* The storage already includes the null terminator */
Containers::String label{NoInit, std::size_t(size)};
glGetObjectLabel(identifier, name, size+1, nullptr, label.data());
return label;
}
#endif

#ifdef MAGNUM_TARGET_GLES
std::string AbstractObject::getLabelImplementationKhrES(const GLenum identifier, const GLuint name) {
Containers::String AbstractObject::getLabelImplementationKhrES(const GLenum identifier, const GLuint name) {
/* Get label size (w/o null terminator). Specifying 0 as size is not
allowed, thus we pass the maximum instead. */
GLsizei size = 0;
glGetObjectLabelKHR(identifier, name, maxLabelLength(), &size, nullptr);

/* Make place also for the null terminator */
std::string label;
label.resize(size+1);
glGetObjectLabelKHR(identifier, name, size+1, nullptr, &label[0]);

/* Pop null terminator and return the string */
label.resize(size);
/* The storage already includes the null terminator */
Containers::String label{NoInit, std::size_t(size)};
glGetObjectLabelKHR(identifier, name, size+1, nullptr, label.data());
return label;
}
#endif

std::string AbstractObject::getLabelImplementationExt(const GLenum identifier, const GLuint name) {
GLsizei size = 0;
Containers::String AbstractObject::getLabelImplementationExt(const GLenum identifier, const GLuint name) {
const GLenum type = extTypeFromKhrIdentifier(identifier);

/* Get label size (w/o null terminator) */
const GLenum type = extTypeFromKhrIdentifier(identifier);
GLsizei size = 0;
glGetObjectLabelEXT(type, name, 0, &size, nullptr);

/* Make place also for the null terminator */
std::string label;
label.resize(size+1);
glGetObjectLabelEXT(type, name, size+1, nullptr, &label[0]);

/* Pop null terminator and return the string */
label.resize(size);
/* The storage already includes the null terminator */
Containers::String label{NoInit, std::size_t(size)};
glGetObjectLabelEXT(type, name, size+1, nullptr, label.data());
return label;
}
#endif
Expand Down
23 changes: 14 additions & 9 deletions src/Magnum/GL/AbstractObject.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,18 @@
* @brief Class @ref Magnum::GL::AbstractObject
*/

#include <string>
#include <Corrade/Containers/EnumSet.h>

#include "Magnum/Magnum.h"
#include "Magnum/GL/OpenGL.h"
#include "Magnum/GL/visibility.h"

#ifdef MAGNUM_BUILD_DEPRECATED
/* For the label stuff that used to be a std::string. Won't cover all cases but
should be sufficient. */
#include <Corrade/Containers/StringStl.h>
#endif

namespace Magnum { namespace GL {

namespace Implementation { struct DebugState; }
Expand Down Expand Up @@ -112,21 +117,21 @@ class MAGNUM_GL_EXPORT AbstractObject {

private:
#ifndef MAGNUM_TARGET_WEBGL
static MAGNUM_GL_LOCAL void labelImplementationNoOp(GLenum, GLuint, Containers::ArrayView<const char> label);
static MAGNUM_GL_LOCAL void labelImplementationExt(GLenum identifier, GLuint name, Containers::ArrayView<const char> label);
static MAGNUM_GL_LOCAL void labelImplementationNoOp(GLenum, GLuint, Containers::StringView label);
static MAGNUM_GL_LOCAL void labelImplementationExt(GLenum identifier, GLuint name, Containers::StringView label);
#ifndef MAGNUM_TARGET_GLES2
static MAGNUM_GL_LOCAL void labelImplementationKhrDesktopES32(GLenum identifier, GLuint name, Containers::ArrayView<const char> label);
static MAGNUM_GL_LOCAL void labelImplementationKhrDesktopES32(GLenum identifier, GLuint name, Containers::StringView label);
#endif
#ifdef MAGNUM_TARGET_GLES
static MAGNUM_GL_LOCAL void labelImplementationKhrES(GLenum identifier, GLuint name, Containers::ArrayView<const char> label);
static MAGNUM_GL_LOCAL void labelImplementationKhrES(GLenum identifier, GLuint name, Containers::StringView label);
#endif
static MAGNUM_GL_LOCAL std::string getLabelImplementationNoOp(GLenum, GLuint);
static MAGNUM_GL_LOCAL std::string getLabelImplementationExt(GLenum identifier, GLuint name);
static MAGNUM_GL_LOCAL Containers::String getLabelImplementationNoOp(GLenum, GLuint);
static MAGNUM_GL_LOCAL Containers::String getLabelImplementationExt(GLenum identifier, GLuint name);
#ifndef MAGNUM_TARGET_GLES2
static MAGNUM_GL_LOCAL std::string getLabelImplementationKhrDesktopES32(GLenum identifier, GLuint name);
static MAGNUM_GL_LOCAL Containers::String getLabelImplementationKhrDesktopES32(GLenum identifier, GLuint name);
#endif
#ifdef MAGNUM_TARGET_GLES
static MAGNUM_GL_LOCAL std::string getLabelImplementationKhrES(GLenum identifier, GLuint name);
static MAGNUM_GL_LOCAL Containers::String getLabelImplementationKhrES(GLenum identifier, GLuint name);
#endif
#endif
};
Expand Down
7 changes: 5 additions & 2 deletions src/Magnum/GL/AbstractQuery.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@

#include "AbstractQuery.h"

#ifndef MAGNUM_TARGET_WEBGL
#include <Corrade/Containers/String.h>
#endif
#include <Corrade/Utility/Assert.h>

#include "Magnum/GL/Context.h"
Expand Down Expand Up @@ -91,15 +94,15 @@ void AbstractQuery::createImplementationDSAExceptPipelineStats() {
#endif

#ifndef MAGNUM_TARGET_WEBGL
std::string AbstractQuery::label() const {
Containers::String AbstractQuery::label() const {
#ifndef MAGNUM_TARGET_GLES2
return Context::current().state().debug.getLabelImplementation(GL_QUERY, _id);
#else
return Context::current().state().debug.getLabelImplementation(GL_QUERY_KHR, _id);
#endif
}

AbstractQuery& AbstractQuery::setLabelInternal(const Containers::ArrayView<const char> label) {
AbstractQuery& AbstractQuery::setLabel(const Containers::StringView label) {
#ifndef MAGNUM_TARGET_GLES2
Context::current().state().debug.labelImplementation(GL_QUERY, _id, label);
#else
Expand Down
22 changes: 9 additions & 13 deletions src/Magnum/GL/AbstractQuery.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
* @brief Class @ref Magnum::GL::AbstractQuery
*/

#include <utility> /* std::swap() */
#include <Corrade/Containers/ArrayView.h>
#include <Corrade/Utility/Assert.h>

Expand All @@ -37,6 +38,12 @@

#include "Magnum/configure.h"

#ifdef MAGNUM_BUILD_DEPRECATED
/* For label() / setLabel(), which used to be a std::string. Not ideal for the
return type, but at least something. */
#include <Corrade/Containers/StringStl.h>
#endif

namespace Magnum { namespace GL {

namespace Implementation { struct QueryState; }
Expand Down Expand Up @@ -93,7 +100,7 @@ class MAGNUM_GL_EXPORT AbstractQuery: public AbstractObject {
* @def_gl{QUERY_OBJECT_EXT}
* @requires_gles Debug output is not available in WebGL.
*/
std::string label() const;
Containers::String label() const;

/**
* @brief Set query label
Expand All @@ -108,14 +115,7 @@ class MAGNUM_GL_EXPORT AbstractQuery: public AbstractObject {
* with @def_gl{QUERY_OBJECT_EXT}
* @requires_gles Debug output is not available in WebGL.
*/
AbstractQuery& setLabel(const std::string& label) {
return setLabelInternal({label.data(), label.size()});
}

/** @overload */
template<std::size_t size> AbstractQuery& setLabel(const char(&label)[size]) {
return setLabelInternal({label, size - 1});
}
AbstractQuery& setLabel(Containers::StringView label);
#endif

/**
Expand Down Expand Up @@ -183,10 +183,6 @@ class MAGNUM_GL_EXPORT AbstractQuery: public AbstractObject {
GLenum _target;

private:
#ifndef MAGNUM_TARGET_WEBGL
AbstractQuery& setLabelInternal(Containers::ArrayView<const char> label);
#endif

void MAGNUM_GL_LOCAL createImplementationDefault();
#ifndef MAGNUM_TARGET_GLES
void MAGNUM_GL_LOCAL createImplementationDSA();
Expand Down
7 changes: 5 additions & 2 deletions src/Magnum/GL/AbstractShaderProgram.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@

#include <Corrade/Containers/Array.h>
#include <Corrade/Containers/StridedArrayView.h>
#ifndef MAGNUM_TARGET_WEBGL
#include <Corrade/Containers/String.h>
#endif
#include <Corrade/Containers/Reference.h>
#include <Corrade/Utility/DebugStl.h>

Expand Down Expand Up @@ -315,15 +318,15 @@ AbstractShaderProgram& AbstractShaderProgram::operator=(AbstractShaderProgram&&
}

#ifndef MAGNUM_TARGET_WEBGL
std::string AbstractShaderProgram::label() const {
Containers::String AbstractShaderProgram::label() const {
#ifndef MAGNUM_TARGET_GLES2
return Context::current().state().debug.getLabelImplementation(GL_PROGRAM, _id);
#else
return Context::current().state().debug.getLabelImplementation(GL_PROGRAM_KHR, _id);
#endif
}

AbstractShaderProgram& AbstractShaderProgram::setLabelInternal(const Containers::ArrayView<const char> label) {
AbstractShaderProgram& AbstractShaderProgram::setLabel(const Containers::StringView label) {
#ifndef MAGNUM_TARGET_GLES2
Context::current().state().debug.labelImplementation(GL_PROGRAM, _id, label);
#else
Expand Down
20 changes: 7 additions & 13 deletions src/Magnum/GL/AbstractShaderProgram.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@
#include <vector>
#endif

#ifdef MAGNUM_BUILD_DEPRECATED
/* For label() / setLabel(), which used to be a std::string */
#include <Corrade/Containers/StringStl.h>
#endif

namespace Magnum { namespace GL {

namespace Implementation { struct ShaderProgramState; }
Expand Down Expand Up @@ -726,7 +731,7 @@ class MAGNUM_GL_EXPORT AbstractShaderProgram: public AbstractObject {
* @def_gl{PROGRAM_OBJECT_EXT}
* @requires_gles Debug output is not available in WebGL.
*/
std::string label() const;
Containers::String label() const;

/**
* @brief Set shader program label
Expand All @@ -741,14 +746,7 @@ class MAGNUM_GL_EXPORT AbstractShaderProgram: public AbstractObject {
* with @def_gl{PROGRAM_OBJECT_EXT}
* @requires_gles Debug output is not available in WebGL.
*/
AbstractShaderProgram& setLabel(const std::string& label) {
return setLabelInternal({label.data(), label.size()});
}

/** @overload */
template<std::size_t size> AbstractShaderProgram& setLabel(const char (&label)[size]) {
return setLabelInternal({label, size - 1});
}
AbstractShaderProgram& setLabel(Containers::StringView label);
#endif

/**
Expand Down Expand Up @@ -1647,10 +1645,6 @@ class MAGNUM_GL_EXPORT AbstractShaderProgram: public AbstractObject {
#endif

private:
#ifndef MAGNUM_TARGET_WEBGL
AbstractShaderProgram& setLabelInternal(Containers::ArrayView<const char> label);
#endif

void bindAttributeLocationInternal(UnsignedInt location, Containers::ArrayView<const char> name);
#ifndef MAGNUM_TARGET_GLES
void bindFragmentDataLocationIndexedInternal(UnsignedInt location, UnsignedInt index, Containers::ArrayView<const char> name);
Expand Down
7 changes: 5 additions & 2 deletions src/Magnum/GL/AbstractTexture.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@

#include <tuple>
#include <Corrade/Containers/Array.h>
#ifndef MAGNUM_TARGET_WEBGL
#include <Corrade/Containers/String.h>
#endif

#include "Magnum/Image.h"
#include "Magnum/ImageView.h"
Expand Down Expand Up @@ -261,12 +264,12 @@ void AbstractTexture::createIfNotAlready() {
}

#ifndef MAGNUM_TARGET_WEBGL
std::string AbstractTexture::label() {
Containers::String AbstractTexture::label() {
createIfNotAlready();
return Context::current().state().debug.getLabelImplementation(GL_TEXTURE, _id);
}

AbstractTexture& AbstractTexture::setLabelInternal(const Containers::ArrayView<const char> label) {
AbstractTexture& AbstractTexture::setLabel(const Containers::StringView label) {
createIfNotAlready();
Context::current().state().debug.labelImplementation(GL_TEXTURE, _id, label);
return *this;
Expand Down
Loading

0 comments on commit bc88442

Please sign in to comment.