checking for memory leaks with valgrind gives errors in simple test when using ClearCache
Elliot
I'm looking at rewriting a libevent/clearsilver app using asio/ctemplate. I'm
also trying to improve my C++ in the process.
I wrote up a simple test of ctemplate, but am getting errors when using valgrind.
If I call ClearCache() and don't free the template, I have the same memory
reachable. If I free the template and do not call ClearCache, I have slightly
more memory that is still reachable.
If I call ClearCache() and free the template, I get a lot of invalid pointer
reads. Are there globals somewhere that are never expected to be free'd up?
Is ClearCache freeing up the Template I've instantiated, and I'm double-freeing it?
I'm running this on x64 ubuntu 8.04, ctemplate 0.90. Please let me know if it
would be better to attach this output as a file.
Thank you in advance for any assistance,
Elliot
------ Source hello_template.cc:
#include <string>
#include <iostream>
#include <google/template.h>
using google::Template;
using google::TemplateDictionary;
void doit() {
google::Template *tpl =
google::Template::GetTemplateWithAutoEscaping("hello_template.tpl",
google::DO_NOT_STRIP, google::TC_HTML);
TemplateDictionary *dict = new TemplateDictionary("hello_template_dict");
dict->SetValue("WHO", "World");
std::string rendered_string;
tpl->Expand(&rendered_string, dict);
std::cout << rendered_string;
//tpl->ClearCache();
delete dict;
delete tpl;
}
int
main (int argc, char **argv) {
doit();
return 0;
}
------ Template 'hello_template.tpl':
<html>
<body>
<h1>Hello, {{WHO}}!</h1>
</body>
</html>
------- Errors:
==24284== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 8 from 1)
--24284--
--24284-- supp: 8 dl-hack3-1
==24284== malloc/free: in use at exit: 2,713 bytes in 32 blocks.
==24284== malloc/free: 145 allocs, 113 frees, 88,196 bytes allocated.
==24284==
==24284== searching for pointers to 32 not-freed blocks.
==24284== checked 198,640 bytes.
==24284==
==24284== 8 bytes in 1 blocks are still reachable in loss record 1 of 12
==24284== at 0x4C23809: operator new(unsigned long) (vg_replace_malloc.c:230)
==24284== by 0x4E4335F: std::vector<google::TemplateDictionary*,
std::allocator<google::TemplateDictionary*>
>::_M_insert_aux(__gnu_cxx::__normal_iterator<google::TemplateDictionary**,
std::vector<google::TemplateDictionary*,
std::allocator<google::TemplateDictionary*> > >, google::TemplateDictionary*
const&) (in /usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E3A40C: google::Template::AssureGlobalsInitialized() (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E3A898: google::Template::template_root_directory() (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E40384: google::Template::GetTemplateCommon(std::string
const&, google::Strip, google::TemplateContext) (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x401049: doit() (hello_template.cc:11)
==24284== by 0x401263: main (hello_template.cc:27)
==24284==
==24284==
==24284== 8 bytes in 1 blocks are still reachable in loss record 2 of 12
==24284== at 0x4C23809: operator new(unsigned long) (vg_replace_malloc.c:230)
==24284== by 0x4E3A3AE: google::Template::AssureGlobalsInitialized() (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E3A898: google::Template::template_root_directory() (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E40384: google::Template::GetTemplateCommon(std::string
const&, google::Strip, google::TemplateContext) (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x401049: doit() (hello_template.cc:11)
==24284== by 0x401263: main (hello_template.cc:27)
==24284==
==24284==
==24284== 24 bytes in 1 blocks are still reachable in loss record 3 of 12
==24284== at 0x4C23809: operator new(unsigned long) (vg_replace_malloc.c:230)
==24284== by 0x4E3A3D6: google::Template::AssureGlobalsInitialized() (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E3A898: google::Template::template_root_directory() (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E40384: google::Template::GetTemplateCommon(std::string
const&, google::Strip, google::TemplateContext) (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x401049: doit() (hello_template.cc:11)
==24284== by 0x401263: main (hello_template.cc:27)
==24284==
==24284==
==24284== 32 bytes in 1 blocks are still reachable in loss record 4 of 12
==24284== at 0x4C23809: operator new(unsigned long) (vg_replace_malloc.c:230)
==24284== by 0x4E39471: (within /usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E40404: google::Template::GetTemplateCommon(std::string
const&, google::Strip, google::TemplateContext) (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x401049: doit() (hello_template.cc:11)
==24284== by 0x401263: main (hello_template.cc:27)
==24284==
==24284==
==24284== 40 bytes in 1 blocks are still reachable in loss record 5 of 12
==24284== at 0x4C23809: operator new(unsigned long) (vg_replace_malloc.c:230)
==24284== by 0x4E462B7: google::TemplateDictionary::SetupGlobalDictUnlocked()
(in /usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E47C8A:
google::TemplateDictionary::TemplateDictionary(std::string const&,
google::UnsafeArena*) (in /usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4010F6: doit() (hello_template.cc:13)
==24284== by 0x401263: main (hello_template.cc:27)
==24284==
==24284==
==24284== 40 bytes in 1 blocks are still reachable in loss record 6 of 12
==24284== at 0x4C23809: operator new(unsigned long) (vg_replace_malloc.c:230)
==24284== by 0x4E40531: google::Template::GetTemplateCommon(std::string
const&, google::Strip, google::TemplateContext) (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x401049: doit() (hello_template.cc:11)
==24284== by 0x401263: main (hello_template.cc:27)
==24284==
==24284==
==24284== 80 bytes in 2 blocks are still reachable in loss record 7 of 12
==24284== at 0x4C23809: operator new(unsigned long) (vg_replace_malloc.c:230)
==24284== by 0x4E4DD0A: __gnu_cxx::hashtable<std::pair<google::TemplateString
const, google::TemplateString>, google::TemplateString,
google::TemplateDictionary::TemplateStringHash,
std::_Select1st<std::pair<google::TemplateString const, google::TemplateString>
>, google::TemplateDictionary::TemplateStringEqual,
std::allocator<google::TemplateString>
>::insert_unique_noresize(std::pair<google::TemplateString const,
google::TemplateString> const&) (in /usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E4DDAD: void
google::TemplateDictionary::HashInsert<google::TemplateString>(__gnu_cxx::hash_map<google::TemplateString,
google::TemplateString, google::TemplateDictionary::TemplateStringHash,
google::TemplateDictionary::TemplateStringEqual,
std::allocator<google::TemplateString> >*, google::TemplateString,
google::TemplateString) (in /usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E4637D: google::TemplateDictionary::SetupGlobalDictUnlocked()
(in /usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E47C8A:
google::TemplateDictionary::TemplateDictionary(std::string const&,
google::UnsafeArena*) (in /usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4010F6: doit() (hello_template.cc:13)
==24284== by 0x401263: main (hello_template.cc:27)
==24284==
==24284==
==24284== 128 bytes in 8 blocks are still reachable in loss record 8 of 12
==24284== at 0x4C23809: operator new(unsigned long) (vg_replace_malloc.c:230)
==24284== by 0x4E447D1:
std::vector<std::pair<google::template_modifiers::ModifierInfo const*,
std::string>, std::allocator<std::pair<google::template_modifiers::ModifierInfo
const*, std::string> >
>::_M_insert_aux(__gnu_cxx::__normal_iterator<std::pair<google::template_modifiers::ModifierInfo
const*, std::string>*,
std::vector<std::pair<google::template_modifiers::ModifierInfo const*,
std::string>, std::allocator<std::pair<google::template_modifiers::ModifierInfo
const*, std::string> > > >, std::pair<google::template_modifiers::ModifierInfo
const*, std::string> const&) (in /usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E3A2E9: (within /usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E3A47C: google::Template::AssureGlobalsInitialized() (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E3A898: google::Template::template_root_directory() (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E40384: google::Template::GetTemplateCommon(std::string
const&, google::Strip, google::TemplateContext) (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x401049: doit() (hello_template.cc:11)
==24284== by 0x401263: main (hello_template.cc:27)
==24284==
==24284==
==24284== 169 bytes in 5 blocks are possibly lost in loss record 9 of 12
==24284== at 0x4C23809: operator new(unsigned long) (vg_replace_malloc.c:230)
==24284== by 0x51042E0: std::string::_Rep::_S_create(unsigned long, unsigned
long, std::allocator<char> const&) (in /usr/lib/libstdc++.so.6.0.9)
==24284== by 0x51050F4: (within /usr/lib/libstdc++.so.6.0.9)
==24284== by 0x5105271: std::string::string(char const*, std::allocator<char>
const&) (in /usr/lib/libstdc++.so.6.0.9)
==24284== by 0x4E3A3C8: google::Template::AssureGlobalsInitialized() (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E3A898: google::Template::template_root_directory() (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E40384: google::Template::GetTemplateCommon(std::string
const&, google::Strip, google::TemplateContext) (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x401049: doit() (hello_template.cc:11)
==24284== by 0x401263: main (hello_template.cc:27)
==24284==
==24284==
==24284== 216 bytes in 9 blocks are still reachable in loss record 10 of 12
==24284== at 0x4C23809: operator new(unsigned long) (vg_replace_malloc.c:230)
==24284== by 0x4E3A429: google::Template::AssureGlobalsInitialized() (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E3A898: google::Template::template_root_directory() (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E40384: google::Template::GetTemplateCommon(std::string
const&, google::Strip, google::TemplateContext) (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x401049: doit() (hello_template.cc:11)
==24284== by 0x401263: main (hello_template.cc:27)
==24284==
==24284==
==24284== 424 bytes in 1 blocks are still reachable in loss record 11 of 12
==24284== at 0x4C23809: operator new(unsigned long) (vg_replace_malloc.c:230)
==24284== by 0x4E4D149:
std::vector<__gnu_cxx::_Hashtable_node<std::pair<google::TemplateString const,
google::TemplateString> >*,
std::allocator<__gnu_cxx::_Hashtable_node<std::pair<google::TemplateString
const, google::TemplateString> >*> >::reserve(unsigned long) (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E4631C: google::TemplateDictionary::SetupGlobalDictUnlocked()
(in /usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E47C8A:
google::TemplateDictionary::TemplateDictionary(std::string const&,
google::UnsafeArena*) (in /usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4010F6: doit() (hello_template.cc:13)
==24284== by 0x401263: main (hello_template.cc:27)
==24284==
==24284==
==24284== 1,544 bytes in 1 blocks are still reachable in loss record 12 of 12
==24284== at 0x4C23809: operator new(unsigned long) (vg_replace_malloc.c:230)
==24284== by 0x4E435F9:
std::vector<__gnu_cxx::_Hashtable_node<std::pair<std::pair<std::string, int>
const, google::Template*> >*,
std::allocator<__gnu_cxx::_Hashtable_node<std::pair<std::pair<std::string, int>
const, google::Template*> >*> >::reserve(unsigned long) (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x4E4059E: google::Template::GetTemplateCommon(std::string
const&, google::Strip, google::TemplateContext) (in
/usr/lib/libctemplate_nothreads.so.0.0.0)
==24284== by 0x401049: doit() (hello_template.cc:11)
==24284== by 0x401263: main (hello_template.cc:27)
==24284==
==24284== LEAK SUMMARY:
==24284== definitely lost: 0 bytes in 0 blocks.
==24284== possibly lost: 169 bytes in 5 blocks.
==24284== still reachable: 2,544 bytes in 27 blocks.
==24284== suppressed: 0 bytes in 0 blocks.
Craig Silverstein
} memory reachable. If I free the template and do not call
} ClearCache, I have slightly more memory that is still reachable.
You should never delete a Template object -- the template system
manages that memory itself. The documentation could probably be
clearer about this.
If you call ClearCache(), all templates are invalidated. So it's no
surprise you run into problems if you call ClearCache() and then try
to operate on a template object (say, by deleting it).
But yes, the template system does allocate some memory for its own
operations, that are never freed. In general, these are global
variables that are allocated lazily -- these are the ones you see
valgrind output pointing to AssureGlobalsInitialized. Likewise, the
template-cache hashtable itself is lazily initialized in
GetTemplateCommon.
I don't use valgrind myself, and the memory checker I do use doesn't
complain about memory that's reachable from global variables at
program-exit time, so I'm not inclined to worry about it myself. If
you'd like to make this valgrind-clean, however, it shouldn't be too
hard. We'd just free these globals in ClearCache() as well. The only
trick is to make sure that any new template instantiation
re-initializes the globals if necessary, which should happen the way
the code is written now.
Personally, since the amount of memory used by these routines is
bounded and small, I wouldn't worry about it.
The only other variable you have to worry about is the global template
dictionary. This dictionary can grow without bound, and is never
cleared. We could make ClearCache() clear the global dictionary, but
that changes the semantics and I'm not sure is a good idea. If you
never insert into the global dictionary via
TemplateDictionary::SetGlobalValue(), then the amount of memory used
here is small and bounded as well.
craig
Elliot
Thank you for the response, Craig.
What memory profiler do you use? I'm not tied to valgrind, it's just what I've
used in the past. I'd be more than open to something else.
I'll take a dive into ClearCache cleaning up template globals. If there is not
a cleanup method for the dictionary-related items, I'll take a whack at creating
one.
Thanks again for your response, and for the library!
Elliot
Craig Silverstein
} just what I've used in the past. I'd be more than open to something
} else.
I use the heap-checker that comes with google-perftools
(http://code.google.com/p/google-perftools). To be fair, it's not
trying to do the same thing valgrind is.
craig
Elliot Foster
> I use the heap-checker that comes with google-perftools
> (http://code.google.com/p/google-perftools). To be fair, it's not
> trying to do the same thing valgrind is.
Very good. It appears I missed many of google's announcements about
open-sourced projects, but finding them now is like Christmas in July!
Here's a first take at cleaning up Template and TemplateDictionary globals
(attached.) Some of it looks a little fishy, but it compiles cleanly and
results in all memory being freed. Let me know if you want anything done
differently.
I'll take a look at perftools as well. It sounds like it's quite a bit faster
than valgrind, which would be very nice.
Elliot
Craig Silverstein
My biggest request is that you update the unittests as well to test
this behavior. In particular, I want to make sure nothing breaks with
usage like this:
<do some template stuff>
Template::ClearCache()
Template* tpl = Template::GetTemplate(...)
<do some stuff on tpl>
Right now, I think this is broken, because you don't reset some
pointers to NULL after clearing them. But overall this looks good.
Feel up to having another go at the patch?
craig
Elliot Foster
I would be happy to.
Elliot
Elliot Foster
Okay, here's another version, but I don't like it. The change you see around
line 36 of src/tests/template_unittest.cc should tell you why.
Though, looking at the documentation for ClearCache, it seems that you are
setting the expectation that ClearCache is only for memory cleanup.
I would be fine with making a separate CleanTemplateGlobals and
CleanTemplateDirectoryGlobals. It would make it very clear what the goal of the
method is, and would leave ClearCache available for folks would like to clear
out their cache every once in a while to free up some memory (but not have to
reconfigure/reinit anything.)
Let me know either way,
Elliot
Craig Silverstein
} see around line 36 of src/tests/template_unittest.cc should tell you
} why.
Right, I understand. :-) I don't know that being able to clear memory
for the template system in this way is that important; I'm certainly
happy to let the OS clean up memory at program-exit, so I wouldn't go
through a lot of effort to make this kind of change just to make
valgrind happy.
On the other hand, if certain applications might 'unload' the template
system before system exit, and really want to reclaim memory if they
do so, a function that does that might make sense. But honestly, I
don't see such a use-case as very likely.
My bias would be to just keep things as they are. If you want, you
could modify ClearCache to also clear values that can be recreated at
need (like g_template_cache) but not worry about the rest (like
template_root_directory_). I'd be fine just leaving things as they
are.
Is there more motivation behind this change than just quieting
valgrind?
craig
Elliot Foster
> My bias would be to just keep things as they are. If you want, you
> could modify ClearCache to also clear values that can be recreated at
> need (like g_template_cache) but not worry about the rest (like
> template_root_directory_). I'd be fine just leaving things as they
> are.
>
> Is there more motivation behind this change than just quieting
> valgrind?
Quieting valgrind, as it's much easier to make sure a line does not appear
(memory leak line does not appear), rather than grabbing the line and parsing it
to make sure the value is not larger than expected (memory leak less than NNNNk.)
I'll make it a separate function to call to clean up globals and resubmit a
patch. That way, if someone really wants to have clean memory, they can do so.
Also, if someone is already calling ClearCache(), there is no need to worry
about its changed behavior.
I would very much like to get it committed, so as to not worry about maintaining
patches for it. Let me know if my proposal above is agreeable to you.
Craig Silverstein
} maintaining patches for it. Let me know if my proposal above is
} agreeable to you.
I'll have to see the patch to see. I'm not a fan of complicating code
just to satisfy an external tool, but if the code makes sense on its
own, that's a different story.
craig