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

Fixed various memory leaks and Java 8 JNI Compatibility #1890

Closed
wants to merge 50 commits into from

Conversation

adamretter
Copy link
Collaborator

@adamretter adamretter commented Feb 21, 2017

I have manually audited the entire RocksJava code base.

Sorry for the large pull-request, I have broken it down into many small atomic commits though.

My initial intention was to fix the warnings that appear when running RocksJava on Java 8 with -Xcheck:jni, for example when running make jtest you would see many errors similar to:

WARNING in native method: JNI call made without checking exceptions when required to from CallObjectMethod
WARNING in native method: JNI call made without checking exceptions when required to from CallVoidMethod
WARNING in native method: JNI call made without checking exceptions when required to from CallStaticVoidMethod
...

A few of those warnings still remain, however they seem to come directly from the JVM and are not directly related to RocksJava; I am in contact with the OpenJDK hostpot-dev mailing list about these - http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-February/025981.html.

As a result of fixing these, I realised we were not really doing any error checking around our JNI calls, which in an unexpected error case could easily put RocksJava into an unknown state and leak memory. I correct this with extensive error checking through the code base.

Along the way I think I found several (11) memory leaks in RocksJava; I believe I have now fixed all of these, the most serious was in org.rocksdb.WBWIRocksIterator#entry.

@adamretter adamretter added bug Confirmed RocksDB bugs java-api labels Feb 21, 2017
@facebook-github-bot
Copy link
Contributor

@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@siying
Copy link
Contributor

siying commented Feb 22, 2017

Is the Java test failure in Travis related?

@adamretter
Copy link
Collaborator Author

@siying I do not think so, I have restarted the job to see. The top of the stack looks like:

C  [librocksdbjni-osx.jnilib+0x16cc65]  _ZN7rocksdb18WriteBatchInternal11SetSequenceEPNS_10WriteBatchEy+0x15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed RocksDB bugs CLA Signed java-api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants