Skip to content

Commit

Permalink
Don't store collection reference in DeckTreeNode
Browse files Browse the repository at this point in the history
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
  • Loading branch information
dae authored and mikehardy committed Jun 29, 2022
1 parent 357a73f commit 477de44
Show file tree
Hide file tree
Showing 11 changed files with 149 additions and 71 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import com.ichi2.libanki.backend.exception.DeckRenameException
import com.ichi2.libanki.exception.EmptyMediaException
import com.ichi2.libanki.sched.AbstractSched
import com.ichi2.libanki.sched.DeckDueTreeNode
import com.ichi2.libanki.sched.TreeNode
import com.ichi2.libanki.utils.TimeManager
import com.ichi2.utils.FileUtil.internalizeUri
import com.ichi2.utils.JSONArray
Expand Down Expand Up @@ -375,28 +376,44 @@ class CardContentProvider : ContentProvider() {
rv
}
DECKS -> {
val allDecks = col.sched.deckDueList()
val columns = projection ?: FlashCardsContract.Deck.DEFAULT_PROJECTION
val rv = MatrixCursor(columns, allDecks.size)
for (deck: DeckDueTreeNode? in allDecks) {
val id = deck!!.did
val name = deck.fullDeckName
addDeckToCursor(id, name, getDeckCountsFromDueTreeNode(deck), rv, col, columns)
val allDecks = col.sched.deckDueTree()
val rv = MatrixCursor(columns, 1)
fun forEach(nodeList: List<TreeNode<DeckDueTreeNode>>, fn: (DeckDueTreeNode) -> Unit) {
for (node in nodeList) {
fn(node.value)
forEach(node.children, fn)
}
}
forEach(allDecks) {
addDeckToCursor(
it.did,
it.fullDeckName,
getDeckCountsFromDueTreeNode(it),
rv,
col,
columns
)
}
rv
}
DECKS_ID -> {

/* Direct access deck */
val columns = projection ?: FlashCardsContract.Deck.DEFAULT_PROJECTION
val rv = MatrixCursor(columns, 1)
val allDecks = col.sched.deckDueList()
val deckId = uri.pathSegments[1].toLong()
for (deck: DeckDueTreeNode? in allDecks) {
if (deck!!.did == deckId) {
addDeckToCursor(deckId, deck.fullDeckName, getDeckCountsFromDueTreeNode(deck), rv, col, columns)
return rv
val allDecks = col.sched.deckDueTree()
val desiredDeckId = uri.pathSegments[1].toLong()
fun find(nodeList: List<TreeNode<DeckDueTreeNode>>, id: Long): DeckDueTreeNode? {
for (node in nodeList) {
if (node.value.did == id) {
return node.value
}
return find(node.children, id)
}
return null
}
find(allDecks, desiredDeckId)?.let {
addDeckToCursor(it.did, it.fullDeckName, getDeckCountsFromDueTreeNode(it), rv, col, columns)
}
rv
}
Expand All @@ -413,10 +430,9 @@ class CardContentProvider : ContentProvider() {
}
}

private fun getDeckCountsFromDueTreeNode(deck: DeckDueTreeNode?): JSONArray {
@KotlinCleanup("use a scope function")
private fun getDeckCountsFromDueTreeNode(deck: DeckDueTreeNode): JSONArray {
val deckCounts = JSONArray()
deckCounts.put(deck!!.lrnCount)
deckCounts.put(deck.lrnCount)
deckCounts.put(deck.revCount)
deckCounts.put(deck.newCount)
return deckCounts
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import java.util.*
* [processChildren] should be called if the children of this node are modified.
*/
abstract class AbstractDeckTreeNode(
val col: Collection,
/**
* @return The full deck name, e.g. "A::B::C"
*/
Expand Down Expand Up @@ -68,7 +67,7 @@ abstract class AbstractDeckTreeNode(
)
}

abstract fun processChildren(children: List<AbstractDeckTreeNode>, addRev: Boolean)
abstract fun processChildren(col: Collection, children: List<AbstractDeckTreeNode>, addRev: Boolean)

override fun toString(): String {
val buf = StringBuffer()
Expand Down
15 changes: 6 additions & 9 deletions AnkiDroid/src/main/java/com/ichi2/libanki/sched/AbstractSched.kt
Original file line number Diff line number Diff line change
Expand Up @@ -175,26 +175,23 @@ abstract class AbstractSched(val col: Collection) {
*/
abstract fun extendLimits(newc: Int, rev: Int)

/**
* @return [deckname, did, rev, lrn, new]
*/
abstract fun deckDueList(): List<DeckDueTreeNode>

/**
* @param cancelListener A task that is potentially cancelled
* @return the due tree. null if task is cancelled
* @return the due tree. null only if task is cancelled
*/
abstract fun deckDueTree(cancelListener: CancelListener?): List<TreeNode<DeckDueTreeNode>>?

/**
* @return the due tree. null if task is cancelled.
* @return the due tree. Never null.
*/
abstract fun deckDueTree(): List<TreeNode<DeckDueTreeNode>>
fun deckDueTree(): List<TreeNode<DeckDueTreeNode>> {
return deckDueTree(null)!!
}

/**
* @return The tree of decks, without numbers
*/
abstract fun quickDeckDueTree(): List<TreeNode<DeckTreeNode>>
abstract fun<T : AbstractDeckTreeNode> quickDeckDueTree(): List<TreeNode<T>>

/** New count for a single deck.
* @param did The deck to consider (descendants and ancestors are ignored)
Expand Down
55 changes: 55 additions & 0 deletions AnkiDroid/src/main/java/com/ichi2/libanki/sched/BackendSched.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/***************************************************************************************
* Copyright (c) 2012 Ankitects Pty Ltd <http://apps.ankiweb.net> *
* *
* This program is free software; you can redistribute it and/or modify it under *
* the terms of the GNU General Public License as published by the Free Software *
* Foundation; either version 3 of the License, or (at your option) any later *
* version. *
* *
* This program is distributed in the hope that it will be useful, but WITHOUT ANY *
* WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A *
* PARTICULAR PURPOSE. See the GNU General Public License for more details. *
* *
* You should have received a copy of the GNU General Public License along with *
* this program. If not, see <http://www.gnu.org/licenses/>. *
****************************************************************************************/

package com.ichi2.libanki.sched

import anki.decks.DeckTreeNode
import com.ichi2.libanki.CollectionV16
import com.ichi2.libanki.utils.TimeManager

// The desktop code stores these routines in sched/base.py, and all schedulers inherit them.
// The presence of AbstractSched is going to complicate the introduction of the v3 scheduler,
// so for now these are stored in a separate file.

fun CollectionV16.deckTree(includeCounts: Boolean): DeckTreeNode {
return backend.deckTree(if (includeCounts) TimeManager.time.intTime() else 0)
}

/**
* Mutate the backend reply into a format expected by legacy code. This is less efficient,
* and AnkiDroid may wish to use .deckTree() in the future instead.
*/
fun CollectionV16.deckTreeLegacy(includeCounts: Boolean): List<TreeNode<DeckDueTreeNode>> {
fun toLegacyNode(node: anki.decks.DeckTreeNode, parentName: String): TreeNode<DeckDueTreeNode> {
val thisName = if (parentName.isEmpty()) {
node.name
} else {
"$parentName::${node.name}"
}
val treeNode = TreeNode(
DeckDueTreeNode(
thisName,
node.deckId,
node.reviewCount,
node.learnCount,
node.newCount,
)
)
treeNode.children.addAll(node.childrenList.asSequence().map { toLegacyNode(it, thisName) })
return treeNode
}
return toLegacyNode(deckTree(includeCounts), "").children
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package com.ichi2.libanki.sched
import com.ichi2.libanki.Collection
import com.ichi2.libanki.Decks
import com.ichi2.utils.KotlinCleanup
import net.ankiweb.rsdroid.RustCleanup
import java.util.*
import kotlin.math.max
import kotlin.math.min
Expand All @@ -34,7 +35,8 @@ import kotlin.math.min
*/
@KotlinCleanup("maybe possible to remove gettres for revCount/lrnCount")
@KotlinCleanup("rename name -> fullDeckName")
class DeckDueTreeNode(col: Collection, name: String, did: Long, override var revCount: Int, override var lrnCount: Int, override var newCount: Int) : AbstractDeckTreeNode(col, name, did) {
@RustCleanup("after migration, consider dropping this and using backend tree structure directly")
class DeckDueTreeNode(name: String, did: Long, override var revCount: Int, override var lrnCount: Int, override var newCount: Int) : AbstractDeckTreeNode(name, did) {
override fun toString(): String {
return String.format(
Locale.US, "%s, %d, %d, %d, %d",
Expand All @@ -50,7 +52,7 @@ class DeckDueTreeNode(col: Collection, name: String, did: Long, override var rev
newCount = max(0, min(newCount, limit))
}

override fun processChildren(children: List<AbstractDeckTreeNode>, addRev: Boolean) {
override fun processChildren(col: Collection, children: List<AbstractDeckTreeNode>, addRev: Boolean) {
// tally up children counts
for (ch in children) {
lrnCount += ch.lrnCount
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@
package com.ichi2.libanki.sched

import com.ichi2.libanki.Collection
import com.ichi2.utils.KotlinCleanup
import net.ankiweb.rsdroid.RustCleanup

@KotlinCleanup("confusing nullability for col, verify real nullability after code related to scheduling is fully migrated to kotlin")
class DeckTreeNode(col: Collection, name: String, did: Long) : AbstractDeckTreeNode(col, name, did) {
override fun processChildren(children: List<AbstractDeckTreeNode>, addRev: Boolean) {
@RustCleanup("processChildren() can be removed after migrating to backend implementation")
class DeckTreeNode(name: String, did: Long) : AbstractDeckTreeNode(name, did) {
override fun processChildren(col: Collection, children: List<AbstractDeckTreeNode>, addRev: Boolean) {
// intentionally blank
}
}
4 changes: 2 additions & 2 deletions AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.java
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ private void unburyCardsForDeck(@NonNull List<Long> allDecks) {
* Returns [deckname, did, rev, lrn, new]
*/
@Override
public @Nullable List<DeckDueTreeNode> deckDueList(@Nullable CancelListener cancelListener) {
protected @Nullable List<DeckDueTreeNode> deckDueList(@Nullable CancelListener cancelListener) {
_checkDay();
getCol().getDecks().checkIntegrity();
List<Deck> allDecksSorted = getCol().getDecks().allSorted();
Expand Down Expand Up @@ -242,7 +242,7 @@ private void unburyCardsForDeck(@NonNull List<Long> allDecks) {
// reviews
int rev = _revForDeck(deck.getLong("id"), rlim);
// save to list
deckNodes.add(new DeckDueTreeNode(getCol(), deck.getString("name"), deck.getLong("id"), rev, lrn, _new));
deckNodes.add(new DeckDueTreeNode(deck.getString("name"), deck.getLong("id"), rev, lrn, _new));
// add deck as a parent
lims.put(Decks.normalizeName(deck.getString("name")), new Integer[]{nlim, rlim});
}
Expand Down
39 changes: 22 additions & 17 deletions AnkiDroid/src/main/java/com/ichi2/libanki/sched/SchedV2.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import android.text.style.StyleSpan;
import android.util.Pair;

import com.ichi2.anki.AnkiDroidApp;
import com.ichi2.anki.R;
import com.ichi2.async.CancelListener;
import com.ichi2.async.CollectionTask;
Expand All @@ -57,6 +58,7 @@
import com.ichi2.utils.JSONObject;
import com.ichi2.utils.SyncStatus;

import net.ankiweb.rsdroid.BackendFactory;
import net.ankiweb.rsdroid.RustCleanup;
import net.ankiweb.rsdroid.RustV1Cleanup;

Expand Down Expand Up @@ -523,14 +525,14 @@ protected int _walkingCount(@NonNull LimitMethod limFn, @NonNull CountMethod cnt
*
* Return nulls when deck task is cancelled.
*/
public @NonNull List<DeckDueTreeNode> deckDueList() {
private @NonNull List<DeckDueTreeNode> deckDueList() {
return deckDueList(null);
}

// Overridden
/**
* Return sorted list of all decks.*/
public @Nullable List<DeckDueTreeNode> deckDueList(@Nullable CancelListener collectionTask) {
protected @Nullable List<DeckDueTreeNode> deckDueList(@Nullable CancelListener collectionTask) {
_checkDay();
getCol().getDecks().checkIntegrity();
List<Deck> allDecksSorted = getCol().getDecks().allSorted();
Expand Down Expand Up @@ -561,7 +563,7 @@ protected int _walkingCount(@NonNull LimitMethod limFn, @NonNull CountMethod cnt
int rlim = _deckRevLimitSingle(deck, plim, false);
int rev = _revForDeck(deck.getLong("id"), rlim, childMap);
// save to list
deckNodes.add(new DeckDueTreeNode(getCol(), deck.getString("name"), deck.getLong("id"), rev, lrn, _new));
deckNodes.add(new DeckDueTreeNode(deck.getString("name"), deck.getLong("id"), rev, lrn, _new));
// add deck as a parent
lims.put(Decks.normalizeName(deck.getString("name")), new Integer[]{nlim, rlim});
}
Expand All @@ -574,32 +576,35 @@ protected int _walkingCount(@NonNull LimitMethod limFn, @NonNull CountMethod cnt
requires multiple database access by deck. Ignoring this number
lead to the creation of a tree more quickly.*/
@Override
public @NonNull List<TreeNode<DeckTreeNode>> quickDeckDueTree() {
// Similar to deckDueTree, ignoring the numbers

public @NonNull
List<? extends TreeNode<? extends AbstractDeckTreeNode>> quickDeckDueTree() {
if (!BackendFactory.getDefaultLegacySchema()) {
return BackendSchedKt.deckTreeLegacy(getCol().getNewBackend(), false);
}
// Similar to deckDueList
ArrayList<DeckTreeNode> allDecksSorted = new ArrayList<>();
for (JSONObject deck : getCol().getDecks().allSorted()) {
DeckTreeNode g = new DeckTreeNode(getCol(), deck.getString("name"), deck.getLong("id"));
DeckTreeNode g = new DeckTreeNode(deck.getString("name"), deck.getLong("id"));
allDecksSorted.add(g);
}
// End of the similar part.

return _groupChildren(allDecksSorted, false);
}


public @NonNull List<TreeNode<DeckDueTreeNode>> deckDueTree() {
return deckDueTree(null);
}

@Nullable
@RustCleanup("enable for v2 once backend is updated to 2.1.41+")
@RustCleanup("once both v1 and v2 are using backend, cancelListener can be removed")
public List<TreeNode<DeckDueTreeNode>> deckDueTree(@Nullable CancelListener cancelListener) {
List<DeckDueTreeNode> allDecksSorted = deckDueList(cancelListener);
if (allDecksSorted == null) {
return null;
if (!BackendFactory.getDefaultLegacySchema()) {
return BackendSchedKt.deckTreeLegacy(getCol().getNewBackend(), true);
} else {
List<DeckDueTreeNode> allDecksSorted = deckDueList(cancelListener);
if (allDecksSorted == null) {
return null;
}
return _groupChildren(allDecksSorted, true);
}
return _groupChildren(allDecksSorted, true);
}

/**
Expand Down Expand Up @@ -666,7 +671,7 @@ public List<TreeNode<DeckDueTreeNode>> deckDueTree(@Nullable CancelListener canc
TreeNode<T> toAdd = new TreeNode<>(child);
toAdd.getChildren().addAll(childrenNode);
List<T> childValues = childrenNode.stream().map(TreeNode::getValue).collect(Collectors.toList());
child.processChildren(childValues, "std".equals(getName()));
child.processChildren(getCol(), childValues, "std".equals(getName()));

sortedChildren.add(toAdd);
}
Expand Down
2 changes: 1 addition & 1 deletion AnkiDroid/src/main/java/com/ichi2/libanki/sync/Syncer.java
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ public JSONObject sanityCheck() {
mCol.getModels().save();
}
// check for missing parent decks
mCol.getSched().deckDueList();
mCol.getSched().quickDeckDueTree();
// return summary of deck
JSONArray check = new JSONArray();
JSONArray counts = new JSONArray();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ public void deckDueTreeInconsistentDecksPasses() {
addDeckWithExactName(child);

getCol().getDecks().checkIntegrity();
assertDoesNotThrow(() -> getCol().getSched().deckDueList());
assertDoesNotThrow(() -> getCol().getSched().deckDueTree());
}


Expand Down
Loading

0 comments on commit 477de44

Please sign in to comment.