symisc / unqlite

An Embedded NoSQL, Transactional Database Engine
https://unqlite.symisc.net
Other
2.11k stars 164 forks source link

Possible memory leak in unqlite_commit() #70

Closed kmvanbrunt closed 6 years ago

kmvanbrunt commented 6 years ago

I am experiencing a memory leak when using unqlite 1.1.9 on Windows. The following code reliably produces this behavior and has been tested on Windows 7 and 10.

Basically I'm noticing that calling unqlite_commit() less often aggravates the memory leak.

The code first writes records to a database and only calls unqlite_commit() once. The memory usage is not reduced by that call to unqlite_commit().

I then close and rewrite the database while calling unqlite_commit() after each write operation. The memory usage after writing all the records is significantly lower that the first approach.

Your team previously fixed a memory leak that I reported in 1.1.8 by adding a call to pager_release_page() in pager_commit_phase1(). Is there a possibility this isn't freeing all pages?

The comments in the code explain all the results I am observing. Hopefully this code helps you track down the issue. Thanks.

#include <stdlib.h>
#include <stdint.h>
#include <stdio.h>
#include <time.h>

#include "unqlite.h"

void randomBytes(uint8_t* bytes, size_t count)
{
    for (size_t i = 0; i < count; i++)
    {
        bytes[i] = (rand() % 256);
    }
}

int main()
{
    char* dbPath = "test.db";
    srand((unsigned int) time(nullptr));

    // Delete any existing database file
    unlink(dbPath);

    // Open the db
    unqlite* db = nullptr;
    if (unqlite_open(&db, dbPath, UNQLITE_OPEN_CREATE) != UNQLITE_OK)
    {
        printf("Error opening %s\n", dbPath);
        return 1;
    }

    // Insert twenty 1 MB records
    size_t numRecords = 20;
    size_t recordSize = 1024 * 1024;
    uint8_t* record = new uint8_t[recordSize];
    randomBytes(record, recordSize);

    for (size_t i = 0; i < numRecords; i++)
    {
        size_t key = i;
        if ((unqlite_kv_store(db, &key, (int) sizeof(key),
                              record, recordSize) != UNQLITE_OK))
        {
            printf("Error saving data\n");
            return 1;
        }
    }

    // At this point on Windows, this process is using about 47 MB of RAM
    // Call commit which should free most of this RAM
    unqlite_commit(db);

    // Still using about 47 MB after commit
    // Free it with close
    unqlite_close(db);

    // Close freed most of the RAM as expected
    // Recreate the DB and this time commit after each call to unqlite_kv_store()
    unlink(dbPath);
    db = nullptr;
    if (unqlite_open(&db, dbPath, UNQLITE_OPEN_CREATE) != UNQLITE_OK)
    {
        printf("Error opening %s\n", dbPath);
        return 1;
    }

    for (size_t i = 0; i < numRecords; i++)
    {
        size_t key = i;
        if ((unqlite_kv_store(db, &key, (int) sizeof(key),
            record, recordSize) != UNQLITE_OK))
        {
            printf("Error saving data\n");
            return 1;
        }

        // Commit each time
        unqlite_commit(db);
    }

    // Now this process is only using about 9 MB of RAM
    // There seems to be a memory leak when committing less often
    // Is unqlite_commit() actually freeing all pages it commits to disk?
    unqlite_close(db);

    delete[] record;
    return 0;
}
symisc commented 6 years ago

Calling unqlite_lib_shutdown() when done should release all memory kept in the global thread pool allocated when the library is first initialized.

-----Original Message----- From: kmvanbrunt notifications@github.com To: symisc/unqlite unqlite@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Sent: Tue, 19 Jun 2018 19:44 Subject: [symisc/unqlite] Possible memory leak in unqlite_commit() (#70)

I am experiencing a memory leak when using unqlite 1.1.9 on Windows. The following code reliably produces this behavior and has been tested on Windows 7 and 10.

Basically I'm noticing that calling unqlite_commit() less often aggravates the memory leak.

The code first writes records to a database and only calls unqlite_commit() once. The memory usage is not reduced by that call to unqlite_commit().

I then close and rewrite the database while calling unqlite_commit() after each write operation. The memory usage after writing all the records is significantly lower that the first approach.

Your team previously fixed a memory leak that I reported in 1.1.8 by adding a call to pager_release_page() in pager_commit_phase1(). Is there a possibility this isn't freeing all pages?

The comments in the code explain all the results I am observing. Hopefully this code helps you track down the issue. Thanks.

#include <stdlib.h>
#include <stdint.h>
#include <stdio.h>
#include <time.h>

#include "unqlite.h"

void randomBytes(uint8_t* bytes, size_t count)
{
    for (size_t i = 0; i < count; i++)
    {
        bytes[i] = (rand() % 256);
    }
}

int main()
{
    char* dbPath = "test.db";
    srand((unsigned int) time(nullptr));

    // Delete any existing database file
    unlink(dbPath);

    // Open the db
    unqlite* db = nullptr;
    if (unqlite_open(&db, dbPath, UNQLITE_OPEN_CREATE) != UNQLITE_OK)
    {
        printf("Error opening %s\n", dbPath);
        return 1;
    }

    // Insert twenty 1 MB records
    size_t numRecords = 20;
    size_t recordSize = 1024 * 1024;
    uint8_t* record = new uint8_t[recordSize];
    randomBytes(record, recordSize);

    for (size_t i = 0; i < numRecords; i++)
    {
        size_t key = i;
        if ((unqlite_kv_store(db, &key, (int) sizeof(key),
                              record, recordSize) != UNQLITE_OK))
        {
            printf("Error saving data\n");
            return 1;
        }
    }

    // At this point on Windows, this process is using about 47 MB of RAM
    // Call commit which should free most of this RAM
    unqlite_commit(db);

    // Still using about 47 MB after commit
    // Free it with close
    unqlite_close(db);

    // Close freed most of the RAM as expected
    // Recreate the DB and this time commit after each call to unqlite_kv_store()
    unlink(dbPath);
    db = nullptr;
    if (unqlite_open(&db, dbPath, UNQLITE_OPEN_CREATE) != UNQLITE_OK)
    {
        printf("Error opening %s\n", dbPath);
        return 1;
    }

    for (size_t i = 0; i < numRecords; i++)
    {
        size_t key = i;
        if ((unqlite_kv_store(db, &key, (int) sizeof(key),
            record, recordSize) != UNQLITE_OK))
        {
            printf("Error saving data\n");
            return 1;
        }

        // Commit each time
        unqlite_commit(db);
    }

    // Now this process is only using about 4.5 MB of RAM
    // There seems to be a memory leak when committing less often
    // Is unqlite_commit() actually freeing all pages it commits to disk?
    unqlite_close(db);

    delete[] record;
    return 0;
}

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/symisc/unqlite/issues/70

symisc commented 6 years ago

Keep in mind that the task of unqlite_commit() is not to free memory but to make sure that everything reaches the surface of the disk. unqlite_close() or unqlite_lib_shutdown() is responsible for freeing memory.

-----Original Message----- From: kmvanbrunt notifications@github.com To: symisc/unqlite unqlite@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Sent: Tue, 19 Jun 2018 19:44 Subject: [symisc/unqlite] Possible memory leak in unqlite_commit() (#70)

I am experiencing a memory leak when using unqlite 1.1.9 on Windows. The following code reliably produces this behavior and has been tested on Windows 7 and 10.

Basically I'm noticing that calling unqlite_commit() less often aggravates the memory leak.

The code first writes records to a database and only calls unqlite_commit() once. The memory usage is not reduced by that call to unqlite_commit().

I then close and rewrite the database while calling unqlite_commit() after each write operation. The memory usage after writing all the records is significantly lower that the first approach.

Your team previously fixed a memory leak that I reported in 1.1.8 by adding a call to pager_release_page() in pager_commit_phase1(). Is there a possibility this isn't freeing all pages?

The comments in the code explain all the results I am observing. Hopefully this code helps you track down the issue. Thanks.

#include <stdlib.h>
#include <stdint.h>
#include <stdio.h>
#include <time.h>

#include "unqlite.h"

void randomBytes(uint8_t* bytes, size_t count)
{
    for (size_t i = 0; i < count; i++)
    {
        bytes[i] = (rand() % 256);
    }
}

int main()
{
    char* dbPath = "test.db";
    srand((unsigned int) time(nullptr));

    // Delete any existing database file
    unlink(dbPath);

    // Open the db
    unqlite* db = nullptr;
    if (unqlite_open(&db, dbPath, UNQLITE_OPEN_CREATE) != UNQLITE_OK)
    {
        printf("Error opening %s\n", dbPath);
        return 1;
    }

    // Insert twenty 1 MB records
    size_t numRecords = 20;
    size_t recordSize = 1024 * 1024;
    uint8_t* record = new uint8_t[recordSize];
    randomBytes(record, recordSize);

    for (size_t i = 0; i < numRecords; i++)
    {
        size_t key = i;
        if ((unqlite_kv_store(db, &key, (int) sizeof(key),
                              record, recordSize) != UNQLITE_OK))
        {
            printf("Error saving data\n");
            return 1;
        }
    }

    // At this point on Windows, this process is using about 47 MB of RAM
    // Call commit which should free most of this RAM
    unqlite_commit(db);

    // Still using about 47 MB after commit
    // Free it with close
    unqlite_close(db);

    // Close freed most of the RAM as expected
    // Recreate the DB and this time commit after each call to unqlite_kv_store()
    unlink(dbPath);
    db = nullptr;
    if (unqlite_open(&db, dbPath, UNQLITE_OPEN_CREATE) != UNQLITE_OK)
    {
        printf("Error opening %s\n", dbPath);
        return 1;
    }

    for (size_t i = 0; i < numRecords; i++)
    {
        size_t key = i;
        if ((unqlite_kv_store(db, &key, (int) sizeof(key),
            record, recordSize) != UNQLITE_OK))
        {
            printf("Error saving data\n");
            return 1;
        }

        // Commit each time
        unqlite_commit(db);
    }

    // Now this process is only using about 4.5 MB of RAM
    // There seems to be a memory leak when committing less often
    // Is unqlite_commit() actually freeing all pages it commits to disk?
    unqlite_close(db);

    delete[] record;
    return 0;
}

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/symisc/unqlite/issues/70

kmvanbrunt commented 6 years ago

I'm a little confused. The documentation for unqlite_commit says to call it at least every 20000 insertions to free memory. Given what the test code shows, this approach would not work. Your answer seems to back this up.

I was under the impression that I could keep a database handle open throughout the life of my application. However, it now sounds like I need to close the file and reopen it periodically to free memory.

Is that the recommended use of unqlite?

symisc commented 6 years ago

unqlite_commit() will free up some memory of course but not everything and will keep up some dirty pages in the cache pool for sure. Your request is to release everything which is impossible since you keep the unqlite handle open that has to deal with the lock table, file handle and other stuff. If you want to free everything, then unqlite_lib_shutdown() or unqlite_close() is your friend.

Symisc

-----Original Message----- From: kmvanbrunt notifications@github.com To: symisc/unqlite unqlite@noreply.github.com Cc: Symisc Systems chm@symisc.net, Comment comment@noreply.github.com Sent: Tue, 19 Jun 2018 20:58 Subject: Re: [symisc/unqlite] Possible memory leak in unqlite_commit() (#70)

I'm a little confused. The documentation for unqlite_commit says to call it at least every 20000 insertions to free memory. Given what the test code shows, this approach would not work. Your answer seems to back this up.

I was under the impression that I could keep a database handle open throughout the life of my application. However, it now sounds like I need to close the file and reopen it periodically to free memory.

Is that the recommended use of unqlite?

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/symisc/unqlite/issues/70#issuecomment-398525590

kmvanbrunt commented 6 years ago

Thank you for the clarification and your quick responses. I will close the issue.