RE: SQLite kind-of memory leak (PATCH) - bug reports

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

RE: SQLite kind-of memory leak (PATCH) - bug reports

Griggs, Donald
 Hi Clifford,

Page http://www.sqlite.org/support.html
Suggests that you *not* directly email the (actually pretty responsive)
author, but instead start a ticket at:

http://www.sqlite.org/cvstrac/tktnew


Donald Griggs


Opinions are not necessarily those of Misys Healthcare Systems nor its board
of directors.


-----Original Message-----
From: Clifford Wolf [mailto:[hidden email]]
Sent: Monday, October 03, 2005 8:00 AM
To: SQLite Users
Cc: [hidden email]
Subject: [sqlite] SQLite kind-of memory leak (PATCH)

Hi,

I have sent the following mail to [hidden email] about a month ago. But I got
no reply and as far as I can see my patch has not been applied in the sqlite
cvs so far.

If [hidden email] is the wrong address to send sqlite development issues,
what is the right one? I can only find a pointer to this 'sqlite users'
list on the webpage.

yours,
 - clifford

On Mon, Sep 12, 2005 at 01:04:13PM +0200, Clifford Wolf wrote:

> Hi,
>
> I'm the developer of a scripting language called SPL. This scripting
> language also has a module for accessing sqlite databases.
>
> I'm using valgrind for checking for memory leaks in SPL. When
> profiling scripts which do access SQLite databases, I've found that
> the lockHash and openHash data structures in os_unix.c don't get freed.
>
> I wouldn't consider that a real memory leak, but it doesn't look nice
> in memory profilers such as valgrind. That's why I recommend the
> attached patch. Please let me know how you think about it..
>
> yours,
>  - clifford

--snip--
Clifford Wolf:
        Fixed kind-of-memory-leak in sqlite-3.2.0 for unix platforms
        (This is not really a memory leak - just unfreed memory which
        is confusing valgrind and other leak checkers..)

--- sqlite-3.2.0/src/os_unix.c 2005-09-09 23:21:57.000000000 +0200
+++ sqlite-3.2.0/src/os_unix.c 2005-09-10 22:38:29.000000000 +0200
@@ -231,6 +231,9 @@
 static Hash lockHash = { SQLITE_HASH_BINARY, 0, 0, 0, 0, 0 };  static Hash
openHash = { SQLITE_HASH_BINARY, 0, 0, 0, 0, 0 };
 
+static int lockHashCounter = 0;
+static int openHashCounter = 0;
+
 
 #ifdef SQLITE_UNIX_THREADS
 /*
@@ -302,6 +305,8 @@
   pLock->nRef--;
   if( pLock->nRef==0 ){
     sqlite3HashInsert(&lockHash, &pLock->key, sizeof(pLock->key), 0);
+    if (--lockHashCounter == 0)
+      sqlite3HashClear(&lockHash);
     sqliteFree(pLock);
   }
 }
@@ -313,6 +318,8 @@
   pOpen->nRef--;
   if( pOpen->nRef==0 ){
     sqlite3HashInsert(&openHash, &pOpen->key, sizeof(pOpen->key), 0);
+    if (--openHashCounter == 0)
+      sqlite3HashClear(&openHash);
     sqliteFree(pOpen->aPending);
     sqliteFree(pOpen);
   }
@@ -360,6 +367,7 @@
     pLock->cnt = 0;
     pLock->locktype = 0;
     pOld = sqlite3HashInsert(&lockHash, &pLock->key, sizeof(key1), pLock);
+    lockHashCounter++;
     if( pOld!=0 ){
       assert( pOld==pLock );
       sqliteFree(pLock);
@@ -383,6 +391,7 @@
     pOpen->nPending = 0;
     pOpen->aPending = 0;
     pOld = sqlite3HashInsert(&openHash, &pOpen->key, sizeof(key2), pOpen);
+    openHashCounter++;
     if( pOld!=0 ){
       assert( pOld==pOpen );
       sqliteFree(pOpen);
--snap--

--
SSSS PPPP L     The SPL Programming Language
S    P  P L     http://www.clifford.at/spl/
SSSS PPPP L     ----------------------------------------------------
   S P    L     An object oriented, stateful, simple, small, c-like,
SSSS P    LLLL  embeddable, feature rich, dynamic scripting language
 
Qrpelcgvat ebg13 ivbyngrf gur QZPN! Cercner gb or fhrq!!
 
Reply | Threaded
Open this post in threaded view
|

Re: RE: SQLite kind-of memory leak (PATCH) - bug reports

D. Richard Hipp
"Griggs, Donald" <[hidden email]> wrote:

> >
> > I'm using valgrind for checking for memory leaks in SPL. When
> > profiling scripts which do access SQLite databases, I've found that
> > the lockHash and openHash data structures in os_unix.c don't get freed.
> >
> > I wouldn't consider that a real memory leak, but it doesn't look nice
> > in memory profilers such as valgrind. That's why I recommend the
> > attached patch. Please let me know how you think about it..
> >
>

You are right: this is not a real memory leak.

I am disinclined to add code to SQLite that serves no purpose
other than to make the output of valgrind look better.  valgrind
is a nice tool for tracking down memory allocation problems.
(SQLite uses a different mechanism, but that should not be
taken as a slight by valgrind.)  But I do not believe that
valgrind should become an end in itself.  I will certainly make
whatever changes to SQLite are necessary to fix *real* memory
leaks.  I would even be willing to modify existing code to
better suit valgrind as long as it doesn't add complexity
or have a run-time cost.  But the changes submitted do have
a run-time cost, and while that cost is very small (perhaps
even unmeasurable) it is finite.  We have worked *very* hard
to remove such minor costs from SQLite and it would be a shame
to add them back, just so that the output of a diagnostic
tool could look nicer.

--
D. Richard Hipp <[hidden email]>

Reply | Threaded
Open this post in threaded view
|

Re: SQLite kind-of memory leak (PATCH) - bug reports

Clifford Wolf
In reply to this post by Griggs, Donald
Hi,

thanks for the quick reply,

On Mon, Oct 03, 2005 at 08:35:10AM -0400, Griggs, Donald wrote:
> Page http://www.sqlite.org/support.html Suggests that you *not* directly
> email the (actually pretty responsive) author,

the page says:

        Please do not send email directly to the author of SQLite
        unless [..] You are working on an open source project.

and there are plenty of open source projects with my name on them, so I
thought it would be ok.

> but instead start a ticket at:
> http://www.sqlite.org/cvstrac/tktnew

what shall I do with the patch? simply copy&paste it to the 'bug description'
text field? i haven't seen a file upload function in the interface.

yours,
 - clifford

--
L I N : B I T           ___
 _______ __ _____  ____|_  |  The OSS cluster synchronization tool for
/ __(_-</ // / _ \/ __/ __/   configuration files an application images
\__/___/\_, /_//_/\__/____/   --- [ http://oss.linbit.com/csync2/ ] ---
       /___/
 
"This 'telephone' has too many shortcomings to be seriously considered as a
means of communication." - Western Union internal memo, 1876.
 
Reply | Threaded
Open this post in threaded view
|

Re: RE: SQLite kind-of memory leak (PATCH) - bug reports

Clifford Wolf
In reply to this post by D. Richard Hipp
Hi,

On Mon, Oct 03, 2005 at 09:02:38AM -0400, [hidden email] wrote:
> You are right: this is not a real memory leak.
> [..]

in fact, for a program which is eg. continously using mktemp() (or a
simmilar but not unsecure api) for creating temporary databases it is a
real memory leak, because the hash table will grow one entry for every
temporary database created.

> [..] just so that the output of a diagnostic tool could look nicer.

what about an sqlite3_cleanup() function which does such cleanups? then it
would add no overhead at all to the library. Right now it is impossible for
a host application to do such cleanups because the allocated memory is only
referenced by static variables in os_unix.c. I could provide a patch which
adds an sqlite3_cleanup() api.

it would even possible to put that code path into a seperate elf section so
it is not paged into the physical memory unless it actually is used by the
application, if the incrased code size would be your next argument. but I
think that this would be overkill.

yours,
 - clifford

--
 ____   ___   ____ _  __  _     _ www.rocklinux.org
|  _ \ / _ \ / ___| |/ / | |   (_)_ __  _   ___  __
| |_) | | | | |   | ' /  | |   | | '_ \| | | \ \/ /
|  _ <| |_| | |___| . \  | |___| | | | | |_| |>  <
|_| \_\\___/ \____|_|\_\ |_____|_|_| |_|\__,_/_/\_\
Reply | Threaded
Open this post in threaded view
|

Re: SQLite kind-of memory leak (PATCH) - bug reports

D. Richard Hipp
In reply to this post by Griggs, Donald
Clifford Wolf <[hidden email]> wrote:
>
> > but instead start a ticket at:
> > http://www.sqlite.org/cvstrac/tktnew
>
> what shall I do with the patch? simply copy&paste it to the 'bug description'
> text field? i haven't seen a file upload function in the interface.
>

There is an "[attach]" hyperlink on the upper right of the ticket
viewer page.
--
D. Richard Hipp <[hidden email]>

Reply | Threaded
Open this post in threaded view
|

RE: RE: SQLite kind-of memory leak (PATCH) - bug reports

Thomas Briggs
In reply to this post by Griggs, Donald

   While I can understand your general sentiment, allowing minor
problems like this to clutter the output from valgrind makes spotting
the real errors amidst the noise more difficult.  Eventually, when
enough of these types of problems exist, valgrind stops being used
altogether, because it's too time consuming to inspect the output.

   In short, I guess my point is that this kind of thing is another
example of "broken window syndrome" - keeping the small things tidy
makes everyone more likely to keep the big things tidy too.

   -Tom

> -----Original Message-----
> From: [hidden email] [mailto:[hidden email]]
> Sent: Monday, October 03, 2005 9:03 AM
> To: [hidden email]
> Subject: Re: RE: [sqlite] SQLite kind-of memory leak (PATCH)
> - bug reports
>
> "Griggs, Donald" <[hidden email]> wrote:
> > >
> > > I'm using valgrind for checking for memory leaks in SPL. When
> > > profiling scripts which do access SQLite databases, I've
> found that
> > > the lockHash and openHash data structures in os_unix.c
> don't get freed.
> > >
> > > I wouldn't consider that a real memory leak, but it
> doesn't look nice
> > > in memory profilers such as valgrind. That's why I recommend the
> > > attached patch. Please let me know how you think about it..
> > >
> >
>
> You are right: this is not a real memory leak.
>
> I am disinclined to add code to SQLite that serves no purpose
> other than to make the output of valgrind look better.  valgrind
> is a nice tool for tracking down memory allocation problems.
> (SQLite uses a different mechanism, but that should not be
> taken as a slight by valgrind.)  But I do not believe that
> valgrind should become an end in itself.  I will certainly make
> whatever changes to SQLite are necessary to fix *real* memory
> leaks.  I would even be willing to modify existing code to
> better suit valgrind as long as it doesn't add complexity
> or have a run-time cost.  But the changes submitted do have
> a run-time cost, and while that cost is very small (perhaps
> even unmeasurable) it is finite.  We have worked *very* hard
> to remove such minor costs from SQLite and it would be a shame
> to add them back, just so that the output of a diagnostic
> tool could look nicer.
>
> --
> D. Richard Hipp <[hidden email]>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RE: SQLite kind-of memory leak (PATCH) - bug reports

D. Richard Hipp
In reply to this post by Griggs, Donald
Clifford Wolf <[hidden email]> wrote:

> Hi,
>
> On Mon, Oct 03, 2005 at 09:02:38AM -0400, [hidden email] wrote:
> > You are right: this is not a real memory leak.
> > [..]
>
> in fact, for a program which is eg. continously using mktemp() (or a
> simmilar but not unsecure api) for creating temporary databases it is a
> real memory leak, because the hash table will grow one entry for every
> temporary database created.
>

I added a new regression test named manydb.test to prove that the
above is not a problem.

I also added code to deallocate the hash tables when their size
reaches zero.  This is pointless code that is there only to make
valgrind happy.  But I get complaints about valgrind frequently
enough that I've grown weary of reading them.  So now SQLite
is a little bigger and a little slower, but at least valgrind
doesn't complain anymore.
--
D. Richard Hipp <[hidden email]>

Reply | Threaded
Open this post in threaded view
|

Re: RE: SQLite kind-of memory leak (PATCH) - bug reports

Clifford Wolf
Hi,

On Mon, Oct 03, 2005 at 11:16:12AM -0400, [hidden email] wrote:
> I also added code to deallocate the hash tables when their size
> reaches zero. [...]

Thanks a lot.

I am sure this is worth the overhead and makes sqlite an even better piece
of software.

yours,
 - clifford

--
 _  _       _      Nerds on Air - Radio for Geeks      _  __     ___
| \| |___  /_\   On 1st and 3rd Friday of the month   / |/ /__  / _ |
| .` / _ \/ _ \    21:00-22:00 CET on Radio Orange   /    / _ \/ __ |
|_|\_\___/_/ \_\     http://www.clifford.at/noa/    /_/|_/\___/_/ |_|
 
perl -le '$_=1;(1x$_)!~/^(11+)\1+$/&&print${_}while$_++<1000'|fmt