google / shaderc

A collection of tools, libraries, and tests for Vulkan shader compilation.
Other
1.82k stars 353 forks source link

Memory Leaks caused by Singletons #356

Open Malacath92 opened 7 years ago

Malacath92 commented 7 years ago

Using shaderc in our code consistently causes ~5.5MB of leaked memory. The issue seems to stem from singletons in shaderc_compiler_initialize and some callee in shaderc_compile_into_spv(CompileToSpecifiedOutputType). Subsequent compiler initializations don't cause additional leaks.

I understand that resources will be cleared when exiting the program, but we don't want keep around that memory, the example-usage in shaderc.hpp seem to imply that it wasn't necessary as well. On top of that it shadows our memory leaks since a total of 1252 leaks are caused by shaderc.

See this super simple code example that suffers from the leaks:

#include <iostream>

#include <shaderc\shaderc.hpp>

int main(int argc, char **argv) {
    {
        shaderc_compiler_t compiler = shaderc_compiler_initialize();
        {
            char *shader = "#version 430 void main() {}";
            size_t size = strlen(shader);
            shaderc_compilation_result_t compileResult = shaderc_compile_into_spv(
                compiler, shader, size,
                shaderc_shader_kind::shaderc_glsl_default_fragment_shader, "test.frag", "main", nullptr);
            shaderc_result_release(compileResult);
        } // Leaks 5259.32 KB
        shaderc_compiler_release(compiler);
    } // Leaks 386.32 KB
    return 0;
}
dneto0 commented 7 years ago

I ran your example through valgrind: valgrind --leak-check=full --show-leak-kinds=all

I'm attaching the report here: log.txt

The biggest leak is a glslang data structures (symbol tables?). It could be that Shaderc isn't properly cleaning up glslang upon release, but I haven't verified that. Glslang has some interesting logic to handle table initialization in the presence of threads, and I'm not clear on how all that is cleaned up, or if it can be.

Malacath92 commented 7 years ago

If we simply remove the static allocation of the GlslangInitializer in shaderc_compiler_initialize and rewrite it together with shaderc_compiler_release to:

shaderc_compiler_t shaderc_compiler_initialize() {
  shaderc_compiler_t compiler = new (std::nothrow) shaderc_compiler;
  compiler->initializer = new shaderc_util::GlslangInitializer;;
  return compiler;
}

void shaderc_compiler_release(shaderc_compiler_t compiler) {
    delete compiler->initializer;
    delete compiler;
}

then we can get rid of most of the leaks. Contrary to what I expected it doesn't seem to cause any problems even with multiple compilers being created at the same time or sequentially. However visual leak detector tells us about another 4 leaks, totaling 352 bytes, the call stack that it gives us is not useful though. We will dig into it and see what causes it.

Obviously the above solution is just a hack that might be appropriate for our current situation, but I presume that a more sophisticated solution should be in place eventually.

Edit: I saw the following comment in compiler.h just now:

// TODO(awoloszyn): Once glslang no longer has static global mutable state
//                  remove this class.

so someone is aware of this at least :)

Malacath92 commented 7 years ago

The last four memory leaks happen in PoolAlloc.cpp in InitializeMemoryPools(). The corresponding function FreeGlobalPools() is never called by shaderc. I myself can't find the correct place to call it without causing undefined behaviour. It feels like a race-condition to me but I can't pretend to know what's going on.

Jasper-Bekkers commented 7 years ago

If we simply remove the static allocation of the GlslangInitializer in shaderc_compiler_initialize and rewrite it together with shaderc_compiler_release to:

I would love it if at least this change can make it into mainline. We're running into the same issue.

We're also running into the InitializeMemoryPools leak fwiw.

dneto0 commented 7 years ago

Sorry I haven't looked further at the Shaderc code.

But I see that @johnkslang has investigated various leaks in Glslang, and produced a plan to address them: https://github.com/KhronosGroup/glslang/issues/976

This might affect what we do in Shaderc, as per the TODO comment by awoloszyn.