asg017 / sqlite-vss

A SQLite extension for efficient vector search, based on Faiss!
MIT License
1.59k stars 59 forks source link

#54 - Fixing memory leaks ++ #55

Closed polterguy closed 1 year ago

polterguy commented 1 year ago

I've been working a bit on fixing up memory leaks. I have little to no knowledge about SQLite plugins, so you might want to sanity check my PR carefully, but I know C++ fairly well, and I could find several places you're creating heap memory without deleting it.

I've also simplified the code some few places, using stack objects instead of heap objects where it made sense. Please let me know if I am on track with what I'm doing, at which point I will spend more time on this during the weekend and help you get the memory usage under control :)

ankon commented 1 year ago

We're looking into this library, so I find this PR interesting. Unfortunately the changes also seem to touch the github actions, and there are quite a few whitespace changes that make reading this PR hard.

polterguy commented 1 year ago

I realised the thing had "multiple coding standards", however the important parts of the PR are the places where I've added delete of pointers, etc - However, if you prefer, I can work on another one this weekend, I just have to get my freakin' C++ compiler up running locally ... :/

Which is why I changed the workflows, because executing the workflows was the only means I had to test my code actually ...

Any hints related to MacOS (x86 CPU) would be helpful. Currently I'm at;

make loadable
cmake -B build; make -C build
-- The C compiler identification is Clang 16.0.5
-- The CXX compiler identification is Clang 16.0.5
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - failed
-- Check for working C compiler: /usr/local/opt/llvm/bin/clang
-- Check for working C compiler: /usr/local/opt/llvm/bin/clang - broken
CMake Error at /usr/local/Cellar/cmake/3.26.4/share/cmake/Modules/CMakeTestCCompiler.cmake:67 (message):
  The C compiler

    "/usr/local/opt/llvm/bin/clang"

  is not able to compile a simple test program.

  It fails with the following output:

    Change Dir: /Users/thomashansen/Documents/projects/magic/temp/sqlite-vss/build/CMakeFiles/CMakeScratch/TryCompile-jQ4zRW

    Run Build Command(s):/usr/local/Cellar/cmake/3.26.4/bin/cmake -E env VERBOSE=1 /usr/bin/make -f Makefile cmTC_a3b3c/fast && /Applications/Xcode.app/Contents/Developer/usr/bin/make  -f CMakeFiles/cmTC_a3b3c.dir/build.make CMakeFiles/cmTC_a3b3c.dir/build
    Building C object CMakeFiles/cmTC_a3b3c.dir/testCCompiler.c.o
    /usr/local/opt/llvm/bin/clang   -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk -mmacosx-version-min=11.7 -MD -MT CMakeFiles/cmTC_a3b3c.dir/testCCompiler.c.o -MF CMakeFiles/cmTC_a3b3c.dir/testCCompiler.c.o.d -o CMakeFiles/cmTC_a3b3c.dir/testCCompiler.c.o -c /Users/thomashansen/Documents/projects/magic/temp/sqlite-vss/build/CMakeFiles/CMakeScratch/TryCompile-jQ4zRW/testCCompiler.c
    Linking C executable cmTC_a3b3c
    /usr/local/Cellar/cmake/3.26.4/bin/cmake -E cmake_link_script CMakeFiles/cmTC_a3b3c.dir/link.txt --verbose=1
    /usr/local/opt/llvm/bin/clang  -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk -mmacosx-version-min=11.7 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -L/usr/local/opt/llvm/lib  CMakeFiles/cmTC_a3b3c.dir/testCCompiler.c.o -o cmTC_a3b3c 
    ld: library not found for -lSystem
    clang-16: error: linker command failed with exit code 1 (use -v to see invocation)
    make[2]: *** [cmTC_a3b3c] Error 1
    make[1]: *** [cmTC_a3b3c/fast] Error 2

  CMake will not be able to correctly generate this project.
Call Stack (most recent call first):
  CMakeLists.txt:2 (project)
polterguy commented 1 year ago

We're looking into this library, so I find this PR interesting. Unfortunately the changes also seem to touch the github actions, and there are quite a few whitespace changes that make reading this PR hard.

I've got the compiler working locally. I'll create a cleaner PR, but to be honest with you, the code is a mess of memory leaks and other issues. It will be a "dramatic PR" for these reasons ... :/

asg017 commented 1 year ago

Hey just wanted to comment here: I'll welcome any contributions that cleanup sqlite-vss memory usage, and I'll be tracking it in this issue: #59