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

Implement Rust-based Database Access #8052

Merged
merged 4 commits into from
Jan 26, 2021

Conversation

david-allison
Copy link
Member

@david-allison david-allison commented Jan 25, 2021

Purpose / Description

  • We want to deprecate the AnkiDroid backend codebase (libAnki) and instead use Anki Desktop's code (rslib)
    • This allows us to quickly follow the changes made to Anki Desktop, and ensures feature parity
  • Usage of the Rust Backend requires an open collection
  • An open collection locks the database
  • Therefore the first step in using rslib is to transfer the database access to flow through rslib

Approach

Thoughts

  • Maybe we want performance tests on the database now - maybe later
    • Seems roughly equivalent/slightly faster than requery
    • Probably: Partially because we don't have a JNI boundary hit when we perform get(String|Int etc...)() on a cursor - logic is handled in the Java.

Known issues

  • Harder to mock time-based dependencies (Collection.crt)
  • Debugging over the Java/Rust boundary appears to be extremely difficult
  • openCollection will open the database and create it with schema and indices. This will not happen in the Java side, so the database setup is slightly different.
  • ⚠️ Potential issue with database upgrades to schema 11 if the user is using a really old AnkiDroid - unsure if we should ask the user to use an earlier version of AnkiDroid, or Anki Desktop for this.
  • Nested database cursors are not supported: Support nested database queries Anki-Android-Backend#23
  • Low memory devices do not use limit/offset for database queries: Implement RustSupportSQLiteDatabase:isLowMemory to enable LimitOffsetSQLiteCursor Anki-Android-Backend#14
  • Missing documentation/sources for the main methods: Provide sources for protobuf-generated files in Build Directory Anki-Android-Backend#15
  • Hardcoded page size streaming data from the Rust of 1000 items per page
  • Rust does not stream data from the database (but has no Heap size limit like a Java app does)
  • This increases the APK size (~10MB) due to the size of the .so, improving this currently is beyond my skillset.
    • We'll later be able to remove requery
    • We can consolidate some of our assets
    • Code will be removed from libAnki
    • Far in the future (pie in the sky), we might be able to use libraries linked to the Android framework

How Has This Been Tested?

  • Physical Device, API 21 emulator and API 25 emulator

Learning

Checklist

  • You have not changed whitespace unnecessarily (it makes diffs hard to read)
  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • Your code follows the style of the project (e.g. never omit braces in if statements)
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)

@codecov
Copy link

codecov bot commented Jan 25, 2021

Codecov Report

Merging #8052 (beb354e) into master (df74a96) will increase coverage by 0.03%.
The diff coverage is 74.57%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #8052      +/-   ##
============================================
+ Coverage     36.87%   36.91%   +0.03%     
- Complexity     3734     3751      +17     
============================================
  Files           347      350       +3     
  Lines         35678    35724      +46     
  Branches       4725     4728       +3     
============================================
+ Hits          13157    13186      +29     
- Misses        21015    21033      +18     
+ Partials       1506     1505       -1     
Impacted Files Coverage Δ Complexity Δ
AnkiDroid/src/main/java/com/ichi2/anki/Info.java 2.04% <0.00%> (-0.11%) 2.00 <0.00> (ø)
...va/com/ichi2/libanki/backend/JavaDroidBackend.java 60.00% <60.00%> (ø) 3.00 <3.00> (?)
...va/com/ichi2/libanki/backend/RustDroidBackend.java 62.50% <62.50%> (ø) 3.00 <3.00> (?)
...com/ichi2/libanki/backend/DroidBackendFactory.java 72.72% <72.72%> (ø) 4.00 <4.00> (?)
...id/src/main/java/com/ichi2/libanki/Collection.java 51.92% <75.00%> (-0.62%) 163.00 <2.00> (-1.00)
AnkiDroid/src/main/java/com/ichi2/libanki/DB.java 87.60% <100.00%> (+1.27%) 42.00 <4.00> (+3.00)
...Droid/src/main/java/com/ichi2/libanki/Storage.java 31.83% <100.00%> (+3.53%) 18.00 <5.00> (+4.00)
.../java/com/ichi2/utils/DatabaseChangeDecorator.java 42.68% <0.00%> (-10.98%) 20.00% <0.00%> (-6.00%)
...src/main/java/com/ichi2/anki/CollectionHelper.java 29.31% <0.00%> (-2.59%) 21.00% <0.00%> (ø%)
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df74a96...beb354e. Read the comment docs.

@dae
Copy link
Contributor

dae commented Jan 25, 2021

By "Debugging over the Java/Rust boundary appears to be extremely difficult" I presume you mean the inability to step the Java debugger into Rust code? For issues that depend on internal transient state such as your DB layer, it would probably be helpful for developers to have an easy way to run AnkiDroid with a locally compiled version of the backend, which they can augment with print statements. But I suspect once the initial infrastructure is set up, the majority of issues that will come up will either solely depend on the input to a backend method, or the input combined with the currently open collection. It's not much different to debugging code that calls a web API - if you feed it good data and you get back bad results, you know the issue is not in your own code, and you can then take that input data and debug it in the other project, or report the issue upstream.

@david-allison
Copy link
Member Author

david-allison commented Jan 25, 2021

TL;DR: Agreed with the final point. I doubt there's going to be much debugging required of the library (famous last words).

Println-based debugging may be necessary, but I feel it's YAGNI right now to get a working setup: ankidroid/Anki-Android-Backend#24


By "Debugging over the Java/Rust boundary appears to be extremely difficult" I presume you mean the inability to step the Java debugger into Rust code?

Yep, there's been successful work in getting LLDB working, but it's far from the mixed-mode debugging experience from other toolchains/mixed languages: mozilla/rust-android-gradle#22.

From vague readings about the quality of the debuggers, I'd only consider this as a last resort, println debugging is sadly likely going to be the way to go.

For issues that depend on internal transient state such as your DB layer, it would probably be helpful for developers to have an easy way to run AnkiDroid with a locally compiled version of the backend, which they can augment with print statements.

We're already printing the database queries being sent to the database to the debug logs.

Instructions for a locally compiled backend are already in the PR (I've been doing most of my testing with a locally compiled backend); they could do with a little more polish to save a ~10LOC change while testing under Robolectric.

Haven't implemented printing to logcat from Rust yet, which would be probably be the best form of debugging that we get for a fair while.

But I suspect once the initial infrastructure is set up, the majority of issues that will come up will either solely depend on the input to a backend method, or the input combined with the currently open collection. It's not much different to debugging code that calls a web API - if you feed it good data and you get back bad results, you know the issue is not in your own code, and you can then take that input data and debug it in the other project, or report the issue upstream.

Agreed - I'd be surprised if we find issues with the upstream code as it's battle-tested.

@mikehardy
Copy link
Member

I actually prefer print debugging I find it much faster in combo with a test infrastructure than debugger fiddling personally :-), as long as it's possible it's fine by me.

@mikehardy
Copy link
Member

The list of knowledge is quite humorous to me - you had to learn All The Things to do this thing. Even android dependency publishing, what a PITA!

AnkiDroid/build.gradle Outdated Show resolved Hide resolved
AnkiDroid/build.gradle Outdated Show resolved Hide resolved
@mikehardy
Copy link
Member

I can imagine all tests pass, it looks really close. Most of my comments were about the last percent (or similar) of removing a couple TODOs, bike-shedding on a variable name or annotation etc

This is temporary - we will later require the Rust Backend

But for now, we have a java fallback which will work.

Mostly aimed at handling class: RustBackendFailedException
@david-allison
Copy link
Member Author

Force pushed. Ready for re-review

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Cleaner in a few ways, LGTM - I'll approve but I can't possibly hit the buttons on this one. You should have the honor - merge at will and hit that publish button

What could possibly go wrong

/ducks 🤣 but seriously launch it!

@david-allison
Copy link
Member Author

@david-allison david-allison merged commit b570c4a into ankidroid:master Jan 26, 2021
@david-allison david-allison deleted the rust branch January 26, 2021 04:12
@mikehardy mikehardy added this to the 2.15 release milestone Apr 4, 2021
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.

3 participants