Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Google tests for unit testing jni #36

Merged
merged 41 commits into from
Jun 15, 2021

Conversation

jmazanec15
Copy link
Member

@jmazanec15 jmazanec15 commented Jun 3, 2021

Description

This PR adds google tests for testing the nmslib_wrapper and faiss_wrapper. Additionally, I made some minor stylistic changes to the jni to help testing and clean up. The primary purpose of these tests is to be able to identify potential memory leaks.

In order to mock the JNIUtilInterface, this PR adds MockJNIUtil class built off of gmock. In the constructor of this class, the default behavior is defined. This can be overriden inside individual test cases.

While the tests cover each library wrapper function, they do not yet exercise all possible functionality. In order to keep the scope of this PR focused, I will leave expansion to a future PR.

In order to build tests:

// This will produce the test executable in bin/jni_test
cd jni
cmake .
make

To run the tests:

// Run all tests
bin/jni_test

// Run certain tests
bin/jni_test --gtest_filter=<Test filter name (can include *)>

// Run tests a certain number of times
bin/jni_test --gtest_repeat=X

In order to test memory checks, I ran the tests with valgrind. Here are some of the results:

>> valgrind bin/jni_test
...
==28779== LEAK SUMMARY:
==28779==    definitely lost: 0 bytes in 0 blocks
==28779==    indirectly lost: 0 bytes in 0 blocks
==28779==      possibly lost: 336 bytes in 1 blocks
==28779==    still reachable: 124,880 bytes in 10 blocks

>> valgrind bin/jni_test --gtest_repeat=4
...
==28790== LEAK SUMMARY:
==28790==    definitely lost: 0 bytes in 0 blocks
==28790==    indirectly lost: 0 bytes in 0 blocks
==28790==      possibly lost: 336 bytes in 1 blocks
==28790==    still reachable: 124,880 bytes in 10 blocks

>> valgrind bin/jni_test --gtest_filter=Nmslib*
...
==28865== LEAK SUMMARY:
==28865==    definitely lost: 0 bytes in 0 blocks
==28865==    indirectly lost: 0 bytes in 0 blocks
==28865==      possibly lost: 0 bytes in 0 blocks
==28865==    still reachable: 122,888 bytes in 7 blocks

>> valgrind bin/jni_test --gtest_filter=NmslibInit*
...
==28875== LEAK SUMMARY:
==28875==    definitely lost: 0 bytes in 0 blocks
==28875==    indirectly lost: 0 bytes in 0 blocks
==28875==      possibly lost: 0 bytes in 0 blocks
==28875==    still reachable: 122,888 bytes in 7 blocks

>> valgrind bin/jni_test --gtest_filter=Faiss*
...
==28876== LEAK SUMMARY:
==28876==    definitely lost: 0 bytes in 0 blocks
==28876==    indirectly lost: 0 bytes in 0 blocks
==28876==      possibly lost: 336 bytes in 1 blocks
==28876==    still reachable: 2,000 bytes in 4 blocks

A couple things to note about these results:

  1. A large chunk of still reachable memory comes from the call to the nmslib initlibrary function. This is okay because it only happens once throughout the lifecycle
  2. In the faiss tests, there is some possibly lost and still reachable memory. I need to further investigate this, but will not do so in this PR.

Issues Resolved

#31

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed as per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

jmazanec15 added 30 commits May 24, 2021 15:42
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15 jmazanec15 added the Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. label Jun 3, 2021
@jmazanec15 jmazanec15 requested review from VijayanB and vamshin June 3, 2021 03:42
@dblock
Copy link
Member

dblock commented Jun 3, 2021

This is nice work, I haven't done C/C++ unit testing in a lifetime, great to see how the modern version looks like!

  1. A large chunk of still reachable memory comes from the call to the nmslib initlibrary function. This is okay because it only happens once throughout the lifecycle

Without looking at the code you probably want to find a way to start measuring leaked memory after the init call(s), or explicitly call a function that cleans up what's allocated in the init call.

Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15
Copy link
Member Author

This is nice work, I haven't done C/C++ unit testing in a lifetime, great to see how the modern version looks like!

  1. A large chunk of still reachable memory comes from the call to the nmslib initlibrary function. This is okay because it only happens once throughout the lifecycle

Without looking at the code you probably want to find a way to start measuring leaked memory after the init call(s), or explicitly call a function that cleans up what's allocated in the init call.

The way I confirmed was that I ran valgrind just on the init library call and the still reachable memory matched the value returned when I ran valgrind for all of the nmslib tests. In terms of deallocation, the init library function calls registration function like this. There does not appear to be a way to unregister these spaces or methods.

Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
Copy link
Member

@vamshin vamshin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for adding coverage for c++ code. This also helps with running Valgrind memleak tests.

@jmazanec15 jmazanec15 merged commit 7755830 into opensearch-project:faiss-develop Jun 15, 2021
jmazanec15 added a commit to jmazanec15/k-NN-1 that referenced this pull request Oct 22, 2021
Signed-off-by: Jack Mazanec <jmazane1@nd.edu>
jmazanec15 added a commit that referenced this pull request Oct 22, 2021
Signed-off-by: Jack Mazanec <jmazane1@nd.edu>
martin-gaievski pushed a commit to martin-gaievski/k-NN that referenced this pull request Mar 7, 2022
Signed-off-by: Jack Mazanec <jmazane1@nd.edu>
martin-gaievski pushed a commit to martin-gaievski/k-NN that referenced this pull request Mar 7, 2022
Signed-off-by: Jack Mazanec <jmazane1@nd.edu>
Signed-off-by: Martin Gaievski <gaievski@amazon.com>
martin-gaievski pushed a commit to martin-gaievski/k-NN that referenced this pull request Mar 30, 2022
Signed-off-by: Jack Mazanec <jmazane1@nd.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants