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

Speed up deck list by rendering it with the backend #11599

Closed
wants to merge 5 commits into from

Conversation

dae
Copy link
Contributor

@dae dae commented Jun 11, 2022

This delegates to the backend to render the deck tree, instead of calculating it in Java. On @Arthur-Milchior's large collection with the daily limits turned up, the speed difference in an x86_64 sim is dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

Depends on the following PRs in this repo:

#11579
#11581

And on a new backend release from David, after merging

ankidroid/anki#1

dae added 5 commits June 11, 2022 19:09
Pass it in to processChildren() instead, which is cleaner, and will
work better with the move to a backend method.
Currently limited to V1 due to changes in 2.1.41
This requires david-allison/anki#2 to be merged
into a new AnkiDroid backend release before this can be used.
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.
@dae
Copy link
Contributor Author

dae commented Jun 11, 2022

Once AnkiDroid has moved to V16, the old Java code can be cut: 5fa2b05

@mikehardy
Copy link
Member

Dependent work to integrate the required change in Anki-Android-Backend tracked here ankidroid/Anki-Android-Backend#200

@mikehardy
Copy link
Member

@dae meta comment - thanks a bunch for all the little fixes in here by the way going to do my best to unblock / merge / release all this stuff so any of your time spent is productive as we can obviously really use the extra set of hands in the area

mikehardy added a commit to mikehardy/Anki-Android that referenced this pull request Jun 12, 2022
@mikehardy mikehardy added the Blocked by dependency Currently blocked by some other dependent / related change label Jun 12, 2022
mikehardy added a commit that referenced this pull request Jun 12, 2022
This moves closer to unblocking #11599
@mikehardy
Copy link
Member

This should be rebased, I managed to bull through cherry-picking the patch in a merge-able spot, and figuring out how to publish the backend, and integrated it here #11605 - so now depends on the other PRs but not any other deep infra

does it really depend on those other two PRs? I'm not sure based on my read of those how this one would depend but maybe I'm being dense, seems they are orthogonal in intent? Or does this actually depend on V16 somehow (which seems more than those other 2 PRs do...)

@dae
Copy link
Contributor Author

dae commented Jun 12, 2022

If #11581 is not merged, I think the build will fail as the abstract method is not implemented in the Java backend. The tests are not a hard requirement, but having 0 failing tests makes it easier to see that this PR hasn't introduced failing ones :-)

@mikehardy
Copy link
Member

Ok - I understand the dependency now I think, obvious in hind sight. 2 paths then:

1- low effort / longer time path: leave as is + blocked by dependency on java backend drop PR while I get acrarium (new crash report system) setup and we get a sample to base dropping the backend on just to make sure, probably a week, but deadlines are open source poison so could be who knows
2- untangle the two PRs by altering this one so it is before dropping the backend

I think 1 is fine, I'm not pushing for 2 just saying how it will go since this will sit for a bit then

optimizing for fewer comments vs having this on the right thread I'll say I'm still confused on the V16 test PR, as I had literally zero success with it in "testing v16 mode", I just don't know how you got that one to pass.

@dae
Copy link
Contributor Author

dae commented Jun 12, 2022

No big rush on my end - I've fallen down the rabbit hole of looking into what's involved with updating AnkiDroidBackend to the latest desktop code, which is why I haven't had a chance to try repro the issue you reported yet.

If it's easy, could I trouble you to get Kotlin working on the backend repo? I tried following some instructions online to add the deps/classpath to the build.gradle files, but was still seeing missing symbol errors when I tried converting one of the files to kotlin (from the java files that were trying to access it).

@dae
Copy link
Contributor Author

dae commented Jun 13, 2022

Nevermind, I think I'm close to getting it working now.

@mikehardy
Copy link
Member

I just got our crash report database up so we're back to 💯 after the server compromise, so I can circle back around and focus more on this again in the next couple days myself

thedroiddiv pushed a commit to thedroiddiv/Anki-Android that referenced this pull request Jun 14, 2022
dae added a commit to ankitects/Anki-Android that referenced this pull request Jun 20, 2022
This delegates to the backend to render the deck tree, instead of
calculating it in Java. On @Arthur-Milchior's large collection with the
daily limits turned up, the speed difference in an x86_64 sim is
dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

- Make deckDueList() private; switch callers to use deckDueTree()
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.

Squashed from ankidroid#11599
@dae
Copy link
Contributor Author

dae commented Jun 20, 2022

I've merged this into #11644

@dae dae closed this Jun 20, 2022
dae added a commit to ankitects/Anki-Android that referenced this pull request Jun 21, 2022
This delegates to the backend to render the deck tree, instead of
calculating it in Java. On @Arthur-Milchior's large collection with the
daily limits turned up, the speed difference in an x86_64 sim is
dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

- Make deckDueList() private; switch callers to use deckDueTree()
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.

Squashed from ankidroid#11599
dae added a commit to ankitects/Anki-Android that referenced this pull request Jun 25, 2022
This delegates to the backend to render the deck tree, instead of
calculating it in Java. On @Arthur-Milchior's large collection with the
daily limits turned up, the speed difference in an x86_64 sim is
dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

- Make deckDueList() private; switch callers to use deckDueTree()
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.

Squashed from ankidroid#11599
dae added a commit to ankitects/Anki-Android that referenced this pull request Jun 25, 2022
This delegates to the backend to render the deck tree, instead of
calculating it in Java. On @Arthur-Milchior's large collection with the
daily limits turned up, the speed difference in an x86_64 sim is
dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

- Make deckDueList() private; switch callers to use deckDueTree()
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.

Squashed from ankidroid#11599
dae added a commit to ankitects/Anki-Android that referenced this pull request Jun 28, 2022
This delegates to the backend to render the deck tree, instead of
calculating it in Java. On @Arthur-Milchior's large collection with the
daily limits turned up, the speed difference in an x86_64 sim is
dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

- Make deckDueList() private; switch callers to use deckDueTree()
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.

Squashed from ankidroid#11599
dae added a commit to ankitects/Anki-Android that referenced this pull request Jun 28, 2022
This delegates to the backend to render the deck tree, instead of
calculating it in Java. On @Arthur-Milchior's large collection with the
daily limits turned up, the speed difference in an x86_64 sim is
dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

- Make deckDueList() private; switch callers to use deckDueTree()
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.

Squashed from ankidroid#11599
dae added a commit to ankitects/Anki-Android that referenced this pull request Jun 28, 2022
This delegates to the backend to render the deck tree, instead of
calculating it in Java. On @Arthur-Milchior's large collection with the
daily limits turned up, the speed difference in an x86_64 sim is
dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

- Make deckDueList() private; switch callers to use deckDueTree()
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.

Squashed from ankidroid#11599
mikehardy pushed a commit that referenced this pull request Jun 29, 2022
This delegates to the backend to render the deck tree, instead of
calculating it in Java. On @Arthur-Milchior's large collection with the
daily limits turned up, the speed difference in an x86_64 sim is
dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

- Make deckDueList() private; switch callers to use deckDueTree()
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.

Squashed from #11599
mikehardy pushed a commit that referenced this pull request Jun 29, 2022
Pass it in to processChildren() instead, which is cleaner, and will
work better with the move to a backend method.

Speed up deck list by rendering it with the backend

This delegates to the backend to render the deck tree, instead of
calculating it in Java. On @Arthur-Milchior's large collection with the
daily limits turned up, the speed difference in an x86_64 sim is
dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

- Make deckDueList() private; switch callers to use deckDueTree()
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.

Squashed from #11599
mikehardy pushed a commit that referenced this pull request Jun 29, 2022
Pass it in to processChildren() instead, which is cleaner, and will
work better with the move to a backend method.

Speed up deck list by rendering it with the backend

This delegates to the backend to render the deck tree, instead of
calculating it in Java. On @Arthur-Milchior's large collection with the
daily limits turned up, the speed difference in an x86_64 sim is
dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

- Make deckDueList() private; switch callers to use deckDueTree()
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.

Squashed from #11599
mikehardy pushed a commit that referenced this pull request Jun 29, 2022
Pass it in to processChildren() instead, which is cleaner, and will
work better with the move to a backend method.

Speed up deck list by rendering it with the backend

This delegates to the backend to render the deck tree, instead of
calculating it in Java. On @Arthur-Milchior's large collection with the
daily limits turned up, the speed difference in an x86_64 sim is
dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

- Make deckDueList() private; switch callers to use deckDueTree()
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.

Squashed from #11599
mikehardy pushed a commit that referenced this pull request Jun 29, 2022
This delegates to the backend to render the deck tree, instead of
calculating it in Java. On @Arthur-Milchior's large collection with the
daily limits turned up, the speed difference in an x86_64 sim is
dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

- Make deckDueList() private; switch callers to use deckDueTree()
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.

Squashed from #11599
mikehardy pushed a commit that referenced this pull request Jun 29, 2022
Pass it in to processChildren() instead, which is cleaner, and will
work better with the move to a backend method.

Speed up deck list by rendering it with the backend

This delegates to the backend to render the deck tree, instead of
calculating it in Java. On @Arthur-Milchior's large collection with the
daily limits turned up, the speed difference in an x86_64 sim is
dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

- Make deckDueList() private; switch callers to use deckDueTree()
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.

Squashed from #11599
mikehardy pushed a commit to ankitects/Anki-Android that referenced this pull request Jun 29, 2022
Pass it in to processChildren() instead, which is cleaner, and will
work better with the move to a backend method.

Speed up deck list by rendering it with the backend

This delegates to the backend to render the deck tree, instead of
calculating it in Java. On @Arthur-Milchior's large collection with the
daily limits turned up, the speed difference in an x86_64 sim is
dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

- Make deckDueList() private; switch callers to use deckDueTree()
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.

Squashed from ankidroid#11599
mikehardy pushed a commit to ankitects/Anki-Android that referenced this pull request Jun 29, 2022
Pass it in to processChildren() instead, which is cleaner, and will
work better with the move to a backend method.

Speed up deck list by rendering it with the backend

This delegates to the backend to render the deck tree, instead of
calculating it in Java. On @Arthur-Milchior's large collection with the
daily limits turned up, the speed difference in an x86_64 sim is
dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

- Make deckDueList() private; switch callers to use deckDueTree()
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.

Squashed from ankidroid#11599
mikehardy pushed a commit that referenced this pull request Jun 29, 2022
Pass it in to processChildren() instead, which is cleaner, and will
work better with the move to a backend method.

Speed up deck list by rendering it with the backend

This delegates to the backend to render the deck tree, instead of
calculating it in Java. On @Arthur-Milchior's large collection with the
daily limits turned up, the speed difference in an x86_64 sim is
dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

- Make deckDueList() private; switch callers to use deckDueTree()
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.

Squashed from #11599
dorrin-sot pushed a commit to dorrin-sot/Anki-Android that referenced this pull request Jul 14, 2022
dorrin-sot pushed a commit to dorrin-sot/Anki-Android that referenced this pull request Jul 14, 2022
Pass it in to processChildren() instead, which is cleaner, and will
work better with the move to a backend method.

Speed up deck list by rendering it with the backend

This delegates to the backend to render the deck tree, instead of
calculating it in Java. On @Arthur-Milchior's large collection with the
daily limits turned up, the speed difference in an x86_64 sim is
dramatic: 2.4s with the old Java code, and 70ms with the new Rust code.

- Make deckDueList() private; switch callers to use deckDueTree()
The backend only provides a deckDueTree(), and switching the calls to
deckDueList() now will make migration easier in the future.

Squashed from ankidroid#11599
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked by dependency Currently blocked by some other dependent / related change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants