-
Notifications
You must be signed in to change notification settings - Fork 27
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
Implement RustSupportSQLiteDatabase:isLowMemory to enable LimitOffsetSQLiteCursor #14
Comments
6 tasks
david-allison
added a commit
that referenced
this issue
Apr 1, 2021
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
david-allison
added a commit
that referenced
this issue
Apr 1, 2021
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
david-allison
added a commit
that referenced
this issue
Apr 3, 2021
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
david-allison
added a commit
that referenced
this issue
Apr 3, 2021
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Requires
ActivityManager
or similar to detect:Which requires a
Context
object.RustSupportSQLiteDatabase
is created byRustSupportSQLiteOpenHelper
.We can inject a
Context
there (or an interface abstracting it) intoBackendFactory
.This feels easy/somewhat reasonable architecturally (only a couple of levels of indirection), but I'll hold off in case there are better options. It feels rather "enterprise" with the layers of abstraction, and there's probably a more clean way.
The text was updated successfully, but these errors were encountered: