Graphical object and memory leaks: root 5 vs root 6

Hello

Sorry to come back on the memory leak, but i’m more than just confused with the latest test I’ve
done recently. It is, in a way, a follow-up of the discussion started about a year ago here:

Sorry also, because this is going to be quite a long post. In the rest of this post I’ll calling
leaks every complains coming from valgrind disregarding whether they’re marked in a suppression file
or not. This is not a criticism, but a shortcut for the sake of simplicity.

To recall, we are relying on ROOT for data handling and some of the graphical aspect and we are
developping another platform on top. We have a large bunch of unitary tests which are run every
night, allowing us to get the coverage of our code and to test the memory leak through valgrind
usage. In order to be efficient, we hope to get as few memory leaks as possible, with a constant
behaviour through time (as long as nothing as changed in our repository and prerequisite). In our
code we are using graphical methods and this was the origin of my question in the post shown above.

I’m currently getting back to these tests now that we are about to switch to root_v6, which is a big
leap (of faith ?) for us. Seeing the results of the valgrind job (sic!) I went back to a dummy
example comparing version 5 (v5.34/36) and 6 (v6.08/00). The dummy code under consideration is the
one below:

#include "TTree.h"
#include "TH1F.h"
#include "TCanvas.h"
#include <iostream>

using namespace std;

enum TOBEDONE{TREE=0, CANVAS, PLOT, HISTO};

int main(int argc, char** argv)
{

    int value=0;   
    if(argc>1)
    {
	value=atoi(argv[1]);
	if( value != 0  && value != 1 && value != 2  && value != 3 && value != 5  && value != 7 && value != 15)
	{
	    cout<<"The command is either 1 for tree creation, 2 for canvas creation, 3 for both."<<endl;
	    cout<<"  5 will draw the tree without canvas while 7 will do everything and 15 will plot in a TH1F"<<endl;
	    return 0;
	}
    }

    TTree *tt = NULL;
    TCanvas *C = NULL;
    TH1F *h=NULL;
    
    if( value & 0x1 << TREE )
    {
	cout<<"create tree"<<endl;
	tt = new TTree("tt","");
    }
    if( value & 0x1 << CANVAS )
    {
	cout<<"create canvas"<<endl;
	C = new TCanvas("C","C",1);
    }
   
    if( value & 0x1 << TREE && value & 0x1 << PLOT )
    {
	cout<<"fill and plot tree"<<endl;
	double pouet, pouet2;
	tt->Branch("pouet",&pouet,"pouet/D");
	tt->Branch("pouet2",&pouet2,"pouet2/D");

	for(unsigned int i=0; i<100; i++)
	{
	    pouet=i;
	    pouet2=float(i)/10;
	    tt->Fill();
	}
	if( value & 0x1 << HISTO )
	{
	    cout<<"create histo"<<endl;
	    h = new TH1F("htest", "", 1000, 0, 150);	
	    tt->Draw("pouet>>htest");
	}
	else
	    tt->Draw("pouet");	
    }


    if( value & 0x1 << TREE && value & 0x1 << PLOT && value & 0x1 << HISTO  )
	delete h;
    
    if( value & 0x1 << CANVAS )
    	delete C;
    
    if( value & 0x1 << TREE )
	delete tt;
}

the concept is that once compiled it can:
** do nothing ==> ARGUMENT = 0
** create a tree (and delete it) ==> ARGUMENT = 1
** create a Canvas (and delete it) ==> ARGUMENT = 2
** create a tree, a Canvas (and delete them) ==> ARGUMENT = 3
** create a tree, fill it and draw the results without canvas creation (and delete it) ==> ARGUMENT = 5
** do everyting ==> ARGUMENT = 7
** do everyting putting the plot in a TH1F ==> ARGUMENT = 15

In order to sum up properly this, I’m using the provided piece of shell-code both for v5 and v6 (it
compiles, run the jobs and dump the logs in output files).

#!/bin/sh

root_version=`root-config --version`
output_file="valgrind_v"
if [[ ${root_version:0:1} == "5" ]] ; then
    
    echo $root_version
    g++ -g -o v5PlottingTest PlottingTest.C `root-config --cflags --evelibs`

    touch $output_file"5.txt"
    for i in {0,1,2,3,5,7,15}
    do
	valgrind --leak-check=full --suppressions=${ROOTSYS}/etc/valgrind-root.supp ./v5PlottingTest $i &>> $output_file"5.txt"
	echo ""&>> $output_file"5.txt"
	echo ""&>> $output_file"5.txt"
    done
	     
elif [[ ${root_version:0:1} == "6" ]] ; then
	 
    echo $root_version
    g++ -g -o v6PlottingTest PlottingTest.C `root-config --cflags --evelibs`

    valgrind --leak-check=full --suppressions=${ROOTSYS}/etc/valgrind-root.supp ./v6PlottingTest $i &> $output_file"6_emptycode.txt"
    
    touch $output_file"6_npl.txt"
    for i in {0,1,2,3,5,7,15}
    do
	valgrind --leak-check=full --show-possibly-lost=no --suppressions=${ROOTSYS}/etc/valgrind-root.supp ./v6PlottingTest $i &>> $output_file"6_npl.txt"
	echo ""&>> $output_file"6_npl.txt"
	echo ""&>> $output_file"6_npl.txt"
    done
fi

The results are joined and discussed here. For root v5, I’m testing all configurations and dumping
the logs in valgrind_v5.txt. There is one stated memory leak when nothing is done at all (it is
removed if the library are not linked in the compilation after commenting out every lines in
connection to ROOT). The results is equivalent if the tree is created and deleted. The first
question I have is when I just create a canvas (ARGUMENT==2): there is 7 leaks popping up. It seems
to me that it complains about the canvas-deletion step, but if I remove this It’ll complain about
not doing it anymore. Finally if one compares the stack when plotting the data with or without the
canvas creation, it leads to the same 9 leaks (which can be reduced to 8 if dumping the plot in an
already created TH1F and deleting it at the end of the job ==> ARGUMENT 15).

**** Do you know where does this behaviour comes from ?

I know that you told me to move to root v6 so this is what I did as well. Running the same macro on
v6, gives me a very (very) large log file (see valgrind_v6_emptycode.txt which is the results of the
empty code, ARGUMENT 0, which is not attached cause considered to big). First question then : are
you aware of that and is this normal ? I’ve run all the configurations discussed above but asking
to remove the possibly lost category to try to see something. This is the other output file
(valgrind_v6_npl.txt) which is way smaller (:D). When doing nothing there are 3 leaks stated but as
soon as some graphical methods are called, there are not there anymore and only a conditionnal jump
statement is made. Same question as before: are you aware of that and is this normal ?

The final question would be more on the stategy to adopt. I have the feeling that I should use the
–show-possibly-lost=no on root v6 but I don’t really like the idea: my main worry is not to hunt
down ROOT memory leak (I trust you to do that more efficiently that I’d ever do) but to find the
ones we are adding on top. I’m a bit worried that using this option, might hide some of our
mistakes. Also I’m wondering whether this option is made just to prevent the dumping on screen of
the leaks under consideration : on the contrary to a suppression file where the leaks are masked
(like in valgrind_v5.txt), the total counter of leaks in valgrind_v6_npl.txt is increasing when
testing the configurations (and the number is huge). Have you changed your strategy concerning the
valgrind suppression file ? What would you recommand ?

Any idea of I might be doing wrong ? Maybe I should not try to delete the canvas (and I"ve tried
many option to do so) but it leads to the same results (in v5).

Once again I’m sorry if this is a messy post, I’ve tried to simplify my problem but I’m pretty sure
I’ve failed.

cheers
jb
valgrind_v6_npl.txt (23.2 KB)
valgrind_v5.txt (70.2 KB)

See also sft.its.cern.ch/jira/browse/ROOT-8408
sft.its.cern.ch/jira/browse/ROOT-8289

Thanks, it seems to go in my direction, but it does not answer the questions i’m asking :stuck_out_tongue:

cheers
jb

Hi,

Indeed, you should always use --show-possibly-lost=no. We intentionally do not delete everything we could during tear-down, for extra stability (mainly in case ROOT is linked into a binary). This suppresses memory regions that are still referenced from a pointer, which is typically not the case for real leaks.

  • The conditional jump could be an error. In v6.08/02 we have already the clang::ASTDeclReader::VisitFriendDecl you also see in your logs. Please report others with reproducer and valgrind log here: root.cern/bugs/

  • Our goal is to add “definitely unreachable” leaks to the valgrind suppression file. If we are missing leaks then that’s likely because we haven’t seen them; please also report those with valgrind log here: root.cern/bugs/

  • The three leaks you report for v6 should indeed be suppressed. We will add them to the suppression file!

  • I don’t think we will revisit the reports for v5.

Cheers, Axel.

Your three leaks are fixed in the master - thanks for the report!
Axel.

thanks you very much for your answer

i’ll use this message to give you few others :

** found in v5.34.36 : in TPie destructor the delete part is incomplete leading to this :

I’ve put these two lines (the for loop) in the if(fNvals), around line 164, recompiled root and it went away

  if (fNvals>0) {
       
       for (int i=0; i<fNvals; ++i) 
	        delete fPieSlices[i];
       
       delete [] fPieSlices;
   }

** This one I haven’t try to correct it When asking for a TTree:Draw with para option, it looks as if it was stacking new TFrames (wild guess this time)

==10522== 208 bytes in 2 blocks are definitely lost in loss record 41,160 of 54,564
==10522==    at 0x4A07117: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==10522==    by 0x5CF3B35: TStorage::ObjectAlloc(unsigned long) (TStorage.cxx:325)
==10522==    by 0x447DEC: TObject::operator new(unsigned long) (TObject.h:156)
==10522==    by 0x863DEE0: TPad::GetFrame() (TPad.cxx:2793)
==10522==    by 0x103845E3: TParallelCoordVar::SetX(double, bool) (TParallelCoordVar.cxx:1024)
==10522==    by 0x1037BA25: TParallelCoord::SetAxesPosition() (TParallelCoord.cxx:941)
==10522==    by 0x10376F54: TParallelCoord::AddVariable(double*, char const*) (TParallelCoord.cxx:183)
==10522==    by 0x103777F8: TParallelCoord::BuildParallelCoord(TSelectorDraw*, bool) (TParallelCoord.cxx:282)
==10522==    by 0x103AD647: G__G__TreeViewer_324_0_11(G__value*, char const*, G__param*, int) (in /export/home/jb232551/RootTarFiles/root/lib/libTreeViewer.so.5.34)
==10522==    by 0x6405F79: Cint::G__ExceptionWrapper(int (*)(G__value*, char const*, G__param*, int), G__value*, char*, G__param*, int) (Api.cxx:393)
==10522==    by 0x6408955: G__execute_call (newlink.cxx:2408)
==10522==    by 0x64092CB: G__call_cppfunc (newlink.cxx:2612)
==10522== 
==10522== 208 bytes in 2 blocks are definitely lost in loss record 41,161 of 54,564
==10522==    at 0x4A07117: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==10522==    by 0x5CF3B35: TStorage::ObjectAlloc(unsigned long) (TStorage.cxx:325)
==10522==    by 0x447DEC: TObject::operator new(unsigned long) (TObject.h:156)
==10522==    by 0x863DEE0: TPad::GetFrame() (TPad.cxx:2793)
==10522==    by 0x103845E3: TParallelCoordVar::SetX(double, bool) (TParallelCoordVar.cxx:1024)
==10522==    by 0x1037B869: TParallelCoord::SetAxesPosition() (TParallelCoord.cxx:932)
==10522==    by 0x10376F54: TParallelCoord::AddVariable(double*, char const*) (TParallelCoord.cxx:183)
==10522==    by 0x103777F8: TParallelCoord::BuildParallelCoord(TSelectorDraw*, bool) (TParallelCoord.cxx:282)
==10522==    by 0x103AD647: G__G__TreeViewer_324_0_11(G__value*, char const*, G__param*, int) (in /export/home/jb232551/RootTarFiles/root/lib/libTreeViewer.so.5.34)
==10522==    by 0x6405F79: Cint::G__ExceptionWrapper(int (*)(G__value*, char const*, G__param*, int), G__value*, char*, G__param*, int) (Api.cxx:393)
==10522==    by 0x6408955: G__execute_call (newlink.cxx:2408)
==10522==    by 0x64092CB: G__call_cppfunc (newlink.cxx:2612)
==10522== 
==10522== 416 bytes in 4 blocks are definitely lost in loss record 54,050 of 54,564
==10522==    at 0x4A07117: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==10522==    by 0x5CF3B35: TStorage::ObjectAlloc(unsigned long) (TStorage.cxx:325)
==10522==    by 0x447DEC: TObject::operator new(unsigned long) (TObject.h:156)
==10522==    by 0x863DEE0: TPad::GetFrame() (TPad.cxx:2793)
==10522==    by 0x1037B37C: TParallelCoord::SetAxesPosition() (TParallelCoord.cxx:909)
==10522==    by 0x10376F54: TParallelCoord::AddVariable(double*, char const*) (TParallelCoord.cxx:183)
==10522==    by 0x103777F8: TParallelCoord::BuildParallelCoord(TSelectorDraw*, bool) (TParallelCoord.cxx:282)
==10522==    by 0x103AD647: G__G__TreeViewer_324_0_11(G__value*, char const*, G__param*, int) (in /export/home/jb232551/RootTarFiles/root/lib/libTreeViewer.so.5.34)
==10522==    by 0x6405F79: Cint::G__ExceptionWrapper(int (*)(G__value*, char const*, G__param*, int), G__value*, char*, G__param*, int) (Api.cxx:393)
==10522==    by 0x6408955: G__execute_call (newlink.cxx:2408)
==10522==    by 0x64092CB: G__call_cppfunc (newlink.cxx:2612)
==10522==    by 0x638612A: G__interpret_func (ifunc.cxx:5791)
==10522== 
==10522== 416 bytes in 4 blocks are definitely lost in loss record 54,051 of 54,564
==10522==    at 0x4A07117: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==10522==    by 0x5CF3B35: TStorage::ObjectAlloc(unsigned long) (TStorage.cxx:325)
==10522==    by 0x447DEC: TObject::operator new(unsigned long) (TObject.h:156)
==10522==    by 0x863DEE0: TPad::GetFrame() (TPad.cxx:2793)
==10522==    by 0x1037B83D: TParallelCoord::SetAxesPosition() (TParallelCoord.cxx:932)
==10522==    by 0x10376F54: TParallelCoord::AddVariable(double*, char const*) (TParallelCoord.cxx:183)
==10522==    by 0x103777F8: TParallelCoord::BuildParallelCoord(TSelectorDraw*, bool) (TParallelCoord.cxx:282)
==10522==    by 0x103AD647: G__G__TreeViewer_324_0_11(G__value*, char const*, G__param*, int) (in /export/home/jb232551/RootTarFiles/root/lib/libTreeViewer.so.5.34)
==10522==    by 0x6405F79: Cint::G__ExceptionWrapper(int (*)(G__value*, char const*, G__param*, int), G__value*, char*, G__param*, int) (Api.cxx:393)
==10522==    by 0x6408955: G__execute_call (newlink.cxx:2408)
==10522==    by 0x64092CB: G__call_cppfunc (newlink.cxx:2612)
==10522==    by 0x638612A: G__interpret_func (ifunc.cxx:5791)
==10522== 
==10522== 1,184 (544 direct, 640 indirect) bytes in 2 blocks are definitely lost in loss record 54,361 of 54,564
==10522==    at 0x4A07117: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==10522==    by 0x5CF3B35: TStorage::ObjectAlloc(unsigned long) (TStorage.cxx:325)
==10522==    by 0x447DEC: TObject::operator new(unsigned long) (TObject.h:156)
==10522==    by 0x10378037: TParallelCoord::Draw(char const*) (TParallelCoord.cxx:389)
==10522==    by 0x10377929: TParallelCoord::BuildParallelCoord(TSelectorDraw*, bool) (TParallelCoord.cxx:289)
==10522==    by 0x103AD647: G__G__TreeViewer_324_0_11(G__value*, char const*, G__param*, int) (in /export/home/jb232551/RootTarFiles/root/lib/libTreeViewer.so.5.34)
==10522==    by 0x6405F79: Cint::G__ExceptionWrapper(int (*)(G__value*, char const*, G__param*, int), G__value*, char*, G__param*, int) (Api.cxx:393)
==10522==    by 0x6408955: G__execute_call (newlink.cxx:2408)
==10522==    by 0x64092CB: G__call_cppfunc (newlink.cxx:2612)
==10522==    by 0x638612A: G__interpret_func (ifunc.cxx:5791)
==10522==    by 0x64A440C: G__getfunction (func.cxx:2660)
==10522==    by 0x64E7508: G__getitem (expr.cxx:1918)
==10522== 
==10522== 42,704 (1,008 direct, 41,696 indirect) bytes in 1 blocks are definitely lost in loss record 54,530 of 54,564
==10522==    at 0x4A07117: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==10522==    by 0x5CF3B35: TStorage::ObjectAlloc(unsigned long) (TStorage.cxx:325)
==10522==    by 0x447DEC: TObject::operator new(unsigned long) (TObject.h:156)
==10522==    by 0xA22F242: TSelectorDraw::Begin(TTree*) (TSelectorDraw.cxx:674)
==10522==    by 0xA20875A: TTreePlayer::Process(TSelector*, char const*, long long, long long) (TTreePlayer.cxx:2117)
==10522==    by 0xA1FFCD1: TTreePlayer::DrawSelect(char const*, char const*, char const*, long long, long long) (TTreePlayer.cxx:409)
==10522==    by 0x89D7CE5: TTree::Draw(char const*, char const*, char const*, long long, long long) (TTree.cxx:4090)
==10522==    by 0x4D1F2D2: URANIE::DataServer::TDataServer::draw(char const*, char const*, char const*, bool) (TDataServer.cxx:5959)
==10522==    by 0x4D24A40: URANIE::DataServer::TDataServer::drawTufte(char const*, char const*, char const*) (TDataServer.cxx:6611)
==10522==    by 0x464405: TDataServerTest::testdrawMethods() (TestTDataServer.cxx:1141)
==10522==    by 0x425319: CppUnit::TestCaller<TDataServerTest>::runTest() (TestCaller.h:166)
==10522==    by 0x34FDC233A1: CppUnit::TestCaseMethodFunctor::operator()() const (in /usr/lib64/libcppunit-1.12.so.1.0.0)
==10522== 
==10522== 42,704 (1,008 direct, 41,696 indirect) bytes in 1 blocks are definitely lost in loss record 54,531 of 54,564
==10522==    at 0x4A07117: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==10522==    by 0x5CF3B35: TStorage::ObjectAlloc(unsigned long) (TStorage.cxx:325)
==10522==    by 0x447DEC: TObject::operator new(unsigned long) (TObject.h:156)
==10522==    by 0xA22F242: TSelectorDraw::Begin(TTree*) (TSelectorDraw.cxx:674)
==10522==    by 0xA20875A: TTreePlayer::Process(TSelector*, char const*, long long, long long) (TTreePlayer.cxx:2117)
==10522==    by 0xA1FFCD1: TTreePlayer::DrawSelect(char const*, char const*, char const*, long long, long long) (TTreePlayer.cxx:409)
==10522==    by 0x89D7CE5: TTree::Draw(char const*, char const*, char const*, long long, long long) (TTree.cxx:4090)

Thanks again for your explanation, We’ll see how to adapt our strategy to this new guidelines.
cheers
jb

adding on top an extra stupid question

the link you provided, is asking for a log in. Once I click on the redirection link, it asks me for my CERN login.
I’m not affiliated to CERN anymore :frowning:… Is there a way to create an account for bug reporting ?

cheers
jb

Hi,

You can create a lightweight CERN account: account.cern.ch/account/Externals/

That allows you to submit new bugs etc.

Cheers, Axel.

You said :

I’ve put these two lines (the for loop) in the if(fNvals), around line 164, recompiled root and it went away

  if (fNvals>0) {
       
       for (int i=0; i<fNvals; ++i) 
           delete fPieSlices[i];
       
       delete [] fPieSlices;
   }

I suppose you did that in TPie ?

I have added these lines in the TPie destructor in the master.
Thanks.