Skip to content

Remove byte array allocation from JNI string methods #564

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

Merged
merged 1 commit into from
Dec 10, 2020

Conversation

tildedave
Copy link
Contributor

Getting string data out of SQLite has involved 2 copies: 1 in JNI-land
(allocating a byte buffer to return to Java) and 1 in Java-land (creating a
String instance).

Rather than do the first copy we can return a direct ByteBuffer instance and
then do the conversion from byte -> String in Java as before. So this removes
a copy getting string data out of SQLite and into Java.

Since these arrays are immediately copied into Java string instances accessing
SQLite memory in Java will be safe. I added a test for a string that used
non-ASCII codepoints just to make sure this was still working as expected.

Getting string data out of SQLite has involved 2 copies: 1 in JNI-land
(allocating a byte buffer to return to Java) and 1 in Java-land (creating a
String instance).

Rather than do the first copy we can return a direct ByteBuffer instance and
then do the conversion from byte -> String in Java as before.  So this removes
a copy getting string data out of SQLite and into Java.

Since these arrays are immediately copied into Java string instances accessing
SQLite memory in Java will be safe.  I added a test for a string that used
non-ASCII codepoints just to make sure this was still working as expected.
@tildedave
Copy link
Contributor Author

Fixes #561 (well, it's much better than existing behavior, at least).

@@ -154,4 +156,18 @@ public void testCloseStatement()

assertTrue(resultSet.isClosed());
}

@Test
public void testReturnsNonAsciiCodepoints()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My changes here didn't break anything, I just noticed there wasn't a test for this.

@xerial xerial self-assigned this Dec 10, 2020
@xerial
Copy link
Owner

xerial commented Dec 10, 2020

Thanks for the quick fix! The code basically looks good to me. I'll double-check whether we need to properly release the allocated memory in sqlite.

@tildedave
Copy link
Contributor Author

Nice, I was working off the guidance here https://sqlite.org/c3ref/column_blob.html which seems to indicate it would handle it until
a type conversion or step() function was called... but there may be more going on. Thanks for the pointers again!

@xerial
Copy link
Owner

xerial commented Dec 10, 2020

@tildedave Thanks. Yeah. In this usage, allocated DirectByteBuffer will be immediately converted into Strings, so it should be safe.

@xerial xerial merged commit 57a676d into xerial:master Dec 10, 2020
@wuhongzhi
Copy link
Contributor

be care for, nativeByteBuffer to string is a slow operation due to the decoder access each byte by JNI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants