Skip to content

Commit

Permalink
feat: fix OOM via memory limit on DB row results
Browse files Browse the repository at this point in the history
We now use a 2MB page size, the same as CursorWindow.sCursorWindowSize

We occasionally got OOMs on methods which returned unbounded data
eg. getting the field data for notes: "Check Database" crashed

To fix this, instead of saying to the Rust "we want 1000 rows max",
we say "we want max 2MB of data"

The calculation for the data is inexact - string length, and 8 bytes
for doubles/longs.
Hasn't been tested thoroughly, but seems to only be < ~7%
off the protobuf size for a string-only column (underestimate).

We measure the rust in-memory usage rather than the size of the
serialized protobuf

Measuring the serialized size wasn't researched, but was assumed to be
hard, as we would need to stream into a protobuf collection, and be
able to efficiently query the new size for each row we add.

Main changes:
StreamingProtobufSQLiteCursor - no longer use pages
dbcommand: allow access via an offset to the result set rather than
via pages

Adds: setDbPageSize to allow the change of the size for debugging
Adds: Unit tests for the rust - not yet executed in CI (#51)
rename: getPage -> getNextSlice

bumps `anki` commit to add field sqlite.proto#DbResponse:start_index

Fixes #14 (no longer necessary)
Fixes ankidroid/#8178
  • Loading branch information
david-allison committed Apr 3, 2021
1 parent 567ab9d commit 03a9d6b
Show file tree
Hide file tree
Showing 12 changed files with 384 additions and 69 deletions.
2 changes: 1 addition & 1 deletion anki
Submodule anki updated from c597f2 to c9e120
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import androidx.sqlite.db.SupportSQLiteDatabase;

import net.ankiweb.rsdroid.ankiutil.DatabaseUtil;
import net.ankiweb.rsdroid.database.StreamingProtobufSQLiteCursor;
import net.ankiweb.rsdroid.database.testutils.DatabaseComparison;

import org.junit.Test;
Expand All @@ -38,6 +37,10 @@
@RunWith(Parameterized.class)
public class DatabaseIntegrationTests extends DatabaseComparison {

private static final int INT_SIZE_BYTES = 8;
private static final int OPTIONAL_BYTES = 1;
/** Number of integers in 1 page of DB results when under test (111) */
public static int DB_PAGE_NUM_INT_ELEMENTS = TEST_PAGE_SIZE / (INT_SIZE_BYTES + OPTIONAL_BYTES);

@Test
public void testScalar() {
Expand Down Expand Up @@ -254,12 +257,12 @@ public void testRowCountPage() {

db.execSQL("create table test (id int)");

for (int i = 0; i < StreamingProtobufSQLiteCursor.RUST_PAGE_SIZE; i++) {
for (int i = 0; i < DB_PAGE_NUM_INT_ELEMENTS; i++) {
db.execSQL("insert into test VALUES (1)");
}

try (Cursor c = db.query("select * from test")) {
assertThat(c.getCount(), is(StreamingProtobufSQLiteCursor.RUST_PAGE_SIZE));
assertThat(c.getCount(), is(DB_PAGE_NUM_INT_ELEMENTS));
}
}

Expand All @@ -269,12 +272,12 @@ public void testRowCountPageAndOne() {

db.execSQL("create table test (id int)");

for (int i = 0; i < StreamingProtobufSQLiteCursor.RUST_PAGE_SIZE + 1; i++) {
for (int i = 0; i < DB_PAGE_NUM_INT_ELEMENTS + 1; i++) {
db.execSQL("insert into test VALUES (1)");
}

try (Cursor c = db.query("select * from test")) {
assertThat(c.getCount(), is(StreamingProtobufSQLiteCursor.RUST_PAGE_SIZE + 1));
assertThat(c.getCount(), is(DB_PAGE_NUM_INT_ELEMENTS + 1));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import net.ankiweb.rsdroid.BackendFactory;
import net.ankiweb.rsdroid.BackendUtils;
import net.ankiweb.rsdroid.BackendV1;
import net.ankiweb.rsdroid.BackendV1Impl;
import net.ankiweb.rsdroid.NativeMethods;
import net.ankiweb.rsdroid.RustBackendFailedException;

import org.junit.After;
Expand All @@ -44,13 +46,22 @@ public class InstrumentedTest {

private final List<BackendV1> backendList = new ArrayList<>();

protected final static int TEST_PAGE_SIZE = 1000;

@Before
public void before() {
/*
Timber added 1 minute to the stress test (1m18 -> 2m30). Didn't seem worth it.
Timber.uprootAll();
Timber.plant(new Timber.DebugTree());
*/

try {
NativeMethods.ensureSetup();
} catch (RustBackendFailedException e) {
throw new RuntimeException(e);
}
BackendV1Impl.setPageSize(TEST_PAGE_SIZE);
}

@After
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,19 @@
import androidx.sqlite.db.SupportSQLiteDatabase;

import net.ankiweb.rsdroid.BackendV1;
import net.ankiweb.rsdroid.DatabaseIntegrationTests;
import net.ankiweb.rsdroid.ankiutil.InstrumentedTest;

import org.junit.Ignore;
import org.junit.Test;

import java.io.IOException;
import java.util.HashSet;
import java.util.Set;

import timber.log.Timber;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.is;
import static org.junit.Assert.fail;

public class StreamingProtobufSQLiteCursorTest extends InstrumentedTest {

Expand Down Expand Up @@ -75,7 +75,7 @@ private void iterateAllRows(SupportSQLiteDatabase db) {

@Test
public void testCorruptionIsHandled() throws IOException {
int elements = StreamingProtobufSQLiteCursor.RUST_PAGE_SIZE;
int elements = DatabaseIntegrationTests.DB_PAGE_NUM_INT_ELEMENTS;

try (BackendV1 backend = super.getBackend("initial_version_2_12_1.anki2")) {
SupportSQLiteDatabase db = new RustSupportSQLiteOpenHelper(backend).getWritableDatabase();
Expand Down Expand Up @@ -109,4 +109,70 @@ public void testCorruptionIsHandled() throws IOException {
}

}

@Test
public void smallQueryHasOneCount() throws IOException {
int elements = 30; // 465


try (BackendV1 backend = super.getBackend("initial_version_2_12_1.anki2")) {
SupportSQLiteDatabase db = new RustSupportSQLiteOpenHelper(backend).getWritableDatabase();

db.execSQL("create table tmp (id varchar)");
for (int i = 0; i < elements + 1; i++) {
String inputOfLength = new String(new char[elements]).replace("\0", "a");
db.execSQL("insert into tmp (id) values (?)", new Object[] {inputOfLength});
}

try (TestCursor c1 = new TestCursor(backend, "select * from tmp", new Object[] { })) {

Set<Integer> sizes = new HashSet<>();

while (c1.moveToNext()) {
if (sizes.add(c1.getSliceSize()) && sizes.size() > 1) {
throw new IllegalStateException("Expected single size of results");
}
}
}
}
}

@Test
public void variableLengthStringsReturnDifferentRowCounts() throws IOException {
int elements = 50; // 1275 > 1000

try (BackendV1 backend = super.getBackend("initial_version_2_12_1.anki2")) {
SupportSQLiteDatabase db = new RustSupportSQLiteOpenHelper(backend).getWritableDatabase();

db.execSQL("create table tmp (id varchar)");
for (int i = 0; i < elements + 1; i++) {
String inputOfLength = new String(new char[elements]).replace("\0", "a");
db.execSQL("insert into tmp (id) values (?)", new Object[] {inputOfLength});
}

try (TestCursor c1 = new TestCursor(backend, "select * from tmp", new Object[] { })) {

Set<Integer> sizes = new HashSet<>();

while (c1.moveToNext()) {
if (sizes.add(c1.getSliceSize()) && sizes.size() > 1) {
return;
}
}

throw new IllegalStateException("Expected multiple sizes of results");
}
}
}

private static class TestCursor extends StreamingProtobufSQLiteCursor {

public TestCursor(SQLHandler backend, String query, Object[] bindArgs) {
super(backend, query, bindArgs);
}

public int getSliceSize() {
return getCurrentSliceRowCount();
}
}
}
4 changes: 2 additions & 2 deletions rsdroid/src/main/java/net/ankiweb/rsdroid/BackendMutex.java
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,10 @@ public String getPath() {
}

@Override
public Sqlite.DBResponse getPage(int page, int sequenceNumber) {
public Sqlite.DBResponse getNextSlice(long startIndex, int sequenceNumber) {
try {
lock.lock();
return backend.getPage(page, sequenceNumber);
return backend.getNextSlice(startIndex, sequenceNumber);
} finally {
lock.unlock();
}
Expand Down
13 changes: 10 additions & 3 deletions rsdroid/src/main/java/net/ankiweb/rsdroid/BackendV1Impl.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import androidx.annotation.CheckResult;
import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting;

import com.google.protobuf.InvalidProtocolBufferException;

Expand Down Expand Up @@ -270,13 +271,13 @@ public Sqlite.DBResponse fullQueryProto(String query, Object... args) {
}

@Override
public Sqlite.DBResponse getPage(int page, int sequenceNumber) {
public Sqlite.DBResponse getNextSlice(long startIndex, int sequenceNumber) {
byte[] result = null;
try {
Timber.d("Rust: getPage %d", page);
Timber.d("Rust: getNextSlice %d", startIndex);

Pointer backend = ensureBackend();
result = NativeMethods.databaseGetNextResultPage(backend.toJni(), sequenceNumber, page);
result = NativeMethods.databaseGetNextResultPage(backend.toJni(), sequenceNumber, startIndex);

Sqlite.DBResponse message = Sqlite.DBResponse.parseFrom(result);
validateMessage(result, message);
Expand Down Expand Up @@ -340,6 +341,12 @@ private void performTransaction(String kind) {
}
}

@VisibleForTesting(otherwise = VisibleForTesting.NONE)
public static void setPageSize(long pageSizeInBytes) {
// TODO: Make this nonstatic
NativeMethods.setDbPageSize(pageSizeInBytes);
}


@Override
public String[] getColumnNames(String sql) {
Expand Down
8 changes: 7 additions & 1 deletion rsdroid/src/main/java/net/ankiweb/rsdroid/NativeMethods.java
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ static void execCommand(long backendPointer, final int command, byte[] args) {
/** Returns the next page of results after a databaseCommand.
* @return DbResult object */
@CheckResult
static native byte[] databaseGetNextResultPage(long backendPointer, int sequenceNumber, int page);
static native byte[] databaseGetNextResultPage(long backendPointer, int sequenceNumber, long startIndex);

/** Clears the memory from the current protobuf query. */
static native int cancelCurrentProtoQuery(long backendPointer, int sequenceNumber);
Expand All @@ -98,6 +98,12 @@ static void execCommand(long backendPointer, final int command, byte[] args) {

static native byte[] executeAnkiDroidCommand(long backendPointer, int command, byte[] args);

/**
* Sets the maximum number of bytes that a page of database results should return
* {@link net.ankiweb.rsdroid.database.StreamingProtobufSQLiteCursor}
*/
static native void setDbPageSize(long numberOfBytes);

/**
* Produces all possible Rust-based errors.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public interface SQLHandler {
String getPath();

/* Protobuf-related (#6) */
Sqlite.DBResponse getPage(int page, int sequenceNumber);
Sqlite.DBResponse getNextSlice(long startIndex, int sequenceNumber);
Sqlite.DBResponse fullQueryProto(String query, Object... bindArgs);

void cancelCurrentProtoQuery(int sequenceNumber);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ public String getPath() {
}

@Override
public Sqlite.DBResponse getPage(int page, int sequenceNumber) {
return backend.getPage(page, sequenceNumber);
public Sqlite.DBResponse getNextSlice(long startIndex, int sequenceNumber) {
return backend.getNextSlice(startIndex, sequenceNumber);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,28 +27,35 @@
import BackendProto.Sqlite;

public class StreamingProtobufSQLiteCursor extends AnkiDatabaseCursor {
// Interleaved cursors would corrupt data if there are more than PAGE_SIZE results.
// We currently use mSequenceNumber to crash if this is the case

// MAINTENANCE: This is not obtained from the Rust, so must manually be kept in sync
public static final int RUST_PAGE_SIZE = 1000;
/**
* Rust Implementation:
*
* When we request a query, rust calculates 2MB (default) of results and sends it to us
*
* We keep track of where we are with getSliceStartIndex: the index into the rust collection
*
* The next request should be for index: getSliceStartIndex() + getCurrentSliceRowCount()
*/

private final SQLHandler backend;
private final String query;
private Sqlite.DBResponse results;
private int position = -1;
private int page = -1;
private String[] columnMapping;
private boolean isClosed = false;
private final int sequenceNumber;
/** The total number of rows for the query */
private final int rowCount;

/**The current index into the collection or rows */
private long getSliceStartIndex() {
return results.getStartIndex();
}

public StreamingProtobufSQLiteCursor(SQLHandler backend, String query, Object[] bindArgs) {
this.backend = backend;
this.query = query;

page++;
try {
results = this.backend.fullQueryProto(this.query, bindArgs);
sequenceNumber = results.getSequenceNumber();
Expand All @@ -59,11 +66,10 @@ public StreamingProtobufSQLiteCursor(SQLHandler backend, String query, Object[]
}

private void getNextPage() {
page++;
position = -1;

try {
results = backend.getPage(page, sequenceNumber);
results = backend.getNextSlice(getSliceStartIndex() + getCurrentSliceRowCount(), sequenceNumber);
if (results.getSequenceNumber() != sequenceNumber) {
throw new IllegalStateException("rsdroid does not currently handle nested cursor-based queries. Please change the code to avoid holding a reference to the query, or implement the functionality in rsdroid");
}
Expand All @@ -79,7 +85,7 @@ public int getCount() {

@Override
public int getPosition() {
return position;
return (int) getSliceStartIndex() + position;
}

@Override
Expand All @@ -93,7 +99,7 @@ public boolean moveToFirst() {

@Override
public boolean moveToNext() {
if (getCurrentSliceRowCount() > 0 && position + 1 >= RUST_PAGE_SIZE && getCount() != RUST_PAGE_SIZE) {
if (getCurrentSliceRowCount() > 0 && position + 1 >= getCurrentSliceRowCount() && getCount() != getCurrentSliceRowCount()) {
getNextPage();
}
position++;
Expand Down Expand Up @@ -250,7 +256,7 @@ private Sqlite.SqlValue getFieldAtIndex(int columnIndex) {
return getRowAtCurrentPosition().getFields(columnIndex);
}

private int getCurrentSliceRowCount() {
protected int getCurrentSliceRowCount() {
return results.getResult().getRowsCount();
}
}
Expand Down
Loading

0 comments on commit 03a9d6b

Please sign in to comment.