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.

Sign up for GitHub

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

Jump to bottom

Get first card quicker during reviews #6035

Merged
merged 6 commits into from
Dec 8, 2020
Merged

Get first card quicker during reviews #6035

merged 6 commits into from
Dec 8, 2020

Conversation

Arthur-Milchior
Copy link
Member

Getting the first card when you select a deck is a long process (at least when you have a big deck collection). This is because before actually getting the first card, AnkiDroid computes the number of new card/review card you need to see. Once it's done, it finally loops over all deck and look for a card to learn.

This PR simply reverse the process. It split reset in two parts, which both can be used or ignored. A part which computes the number of cards to see and a part which reset all queues and list of decks to consider. When reset should be done in getCard, it first runs the important part of reset (i.e. resetting queues), then look for a card, then finally, in background, computes the numbers of cards.

Ideally, I would just separate reset in a pair of functions. I prefer to stay close to upsteam, and so I don't do this and just add variables to control which part of the resetting process I want to deactivate.

This is again a part of #6010

It has a commit in common with #6026 and it contains #6034. Without both of those commits, this PR could not work. Which also means that this PR will get smaller once both other PR are accepted.

@Arthur-Milchior
Copy link
Member Author

Given that CI works and the problem is only the RuntimeException, I won't change this PR unless you know what else can be done to avoid it

@Arthur-Milchior Arthur-Milchior force-pushed the getCardQuicker branch 3 times, most recently from 7059efa to 6445b4a Compare April 17, 2020 16:20
@Arthur-Milchior
Copy link
Member Author

Rebased onto master because one of its commit was merged, which let to a conflict

@Arthur-Milchior Arthur-Milchior force-pushed the getCardQuicker branch 4 times, most recently from fa91112 to d8ba8a7 Compare April 18, 2020 11:05
@Arthur-Milchior
Copy link
Member Author

Rebased on master, because of merge conflict

@Arthur-Milchior
Copy link
Member Author

the tests works on my phone. I have no idea what's wrong with the tests here specifically

Task :AnkiDroid:testDebugUnitTest
com.ichi2.anki.AbstractFlashcardViewerTest > testTypeAnsAnswerFilterNormalEmpty FAILED
Unable to resolve artifact: Missing:
----------
1) org.robolectric:android-all:jar:9-robolectric-4913185-2
Try downloading the file manually from the project website.

@Arthur-Milchior Arthur-Milchior force-pushed the getCardQuicker branch 3 times, most recently from d1c7e02 to 904f94c Compare April 18, 2020 17:06
@Arthur-Milchior Arthur-Milchior changed the title Get card quicker Get first card quicker during reviews Apr 21, 2020
@Arthur-Milchior
Copy link
Member Author

I pushed a new code. That's the same idea behind it, but I fouud a way to make the code cleaner while keeping it as fast

@Arthur-Milchior
Copy link
Member Author

CI failed due to

/home/travis/.jabba/bin/jabba does not appear to be a valid binary.
Check your Internet connection / proxy settings and try again.

I'll ignore this bug until CI/github is repaired

@Arthur-Milchior Arthur-Milchior force-pushed the getCardQuicker branch 3 times, most recently from f303396 to 604d736 Compare April 25, 2020 11:18
@Arthur-Milchior
Copy link
Member Author

Rebased to solve merge conflict

@timrae
Copy link
Member

timrae commented May 14, 2020

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- AnkiDroid/src/main/java/com/ichi2/async/CollectionTask.java  1

See the complete overview on Codacy

@Arthur-Milchior
Copy link
Member Author

Rebased to remove the generic task "RUNNABLE" as requested by @david-allison-1 in another PR. Instead I use a task whose goal is specifically to reset the counts.

@Arthur-Milchior Arthur-Milchior force-pushed the getCardQuicker branch 3 times, most recently from ef4bc4e to 19ef59b Compare June 8, 2020 14:55
@Arthur-Milchior
Copy link
Member Author

Arthur-Milchior commented Jun 8, 2020 •
edited
Loading

Rebased to use the change from #6404

@mikehardy mikehardy added this to the 2.14 release milestone Sep 30, 2020
@Arthur-Milchior Arthur-Milchior force-pushed the getCardQuicker branch 2 times, most recently from d87fc67 to fed080a Compare September 30, 2020 15:04
@Arthur-Milchior
Copy link
Member Author

This is still valid. But it still have the problem to have test errors that are related to background task time conflict. So probably depends on being able to run tests in foreground, which itself depends on having a TaskManager. So I don't expect to have it merged soon.
To be more specific, even if we decide that running all test in foreground is a bad idea, I still believe it would be nice to be able to run some of them without using background.

#7127 will conflict and is probably simpler to consider first.

@Arthur-Milchior
Copy link
Member Author

image
Yeah, as I expected, it fails intermitently.
There is a loop where each occurence call a background process and that does not wait properly for the process to end. That's not even a good simulation of actual use of ankidroid, since we never ask getCard to give us 10 cards immediately one after the other without waiting for the previous card to be computed and shown to the user

@Arthur-Milchior Arthur-Milchior force-pushed the getCardQuicker branch 2 times, most recently from e53e21e to 7fc364f Compare October 15, 2020 06:41
@mikehardy
Copy link
Member

failed unit tests in CI

@Arthur-Milchior
Copy link
Member Author

It works on computer. I fear I've still not added enough advanceRobolectricLooperWithSleep. Because here the problem really comes from the fact that a background task didn't have enough time to ends. Which would be impossible in real use. I try adding more of them. Hopefully, at some point I'll be able to use task manager to avoid this problem

@Arthur-Milchior Arthur-Milchior force-pushed the getCardQuicker branch 2 times, most recently from f597973 to 6cc5af1 Compare October 18, 2020 01:00
@mikehardy mikehardy modified the milestones: 2.14 release, 2.15 release Nov 10, 2020
@Arthur-Milchior
NF: remove _getLrnCard()
62663dd
@Arthur-Milchior
If counts are not corrects, ignore them
6eccc91 
In schedulers, the counts are used for two distinct reasons:
* Showing the actual number to the user in the reviewer/overview
* Winning time by avoiding to compute some part of the queue which are
  known to be empty.

Currently, the queues can't be computed before the count. I expect it
to change in future commit. In order to do it safely, I first ensure
that if the counts are not known, the counts are not
considered. Assuming counts are correct, it does not change anything
in the behaviour of code. The only difference is that we may take time
to check whether we can fill queues while actually counts may have
told us that they were necessarily empty. However, since computing
counts is slower than computing queues, it's still quicker overall.
@Arthur-Milchior
NF: add a background task to reset counts
d5470fb
@Arthur-Milchior
getCard fills queues before counts
b6b45e1 
Currently, if scheduler values are not yet computed (resets) then the
scheduler first compute the counts, and then fills the
queues. This may seems useful since counts are used to win time
computing the queues: i.e. if there are no cards to review, the review
queue won't be filled.

However, counting the number of review is actually slower than just
checking for all subdecks for the first card to review. This means
that actually, computing the queues first while ignoring the counts is
actually faster. It allows to fill the queue and return quicker. A
background tasks can then compute the counts, so that the next
getCards become quicker and the correct number of cards can be
displayed in the reviewer.

Note that the new counts allow also to decide whether to show a new or
a review card when those reviews are mixed. This means that the random
choice of "new card or review card" is potentially changed for the
first card reviewed. As it only change the first card reviewed and
that it still allows the review to alternate between new card and
review card, it seems totally acceptable.
@Arthur-Milchior
Remove recalculate counts
210ff84 
Given that upstream calls `reset` during sync's check, it makes sense
to call it ourselve. Using `resetCount` instead of `recalculateCounts`
let us be closer to upstream.
@mikehardy
Copy link
Member

Well, we don't have Travis right now, and when I pull this branch locally, rebase it against tip of main branch and run it, everything passes 🤷 - we go!

@mikehardy mikehardy merged commit e64c2e3 into ankidroid:master Dec 8, 2020
@mikehardy
Copy link
Member

mikehardy commented Dec 8, 2020 •
edited
Loading

Thanks for your patience on this one - you are very understanding (which I am thankful for) and I just want to say that it is still on me to honor your work by reviewing it and merging it whenever possible. Your contributions are helpful and even though you are understanding I am sad it has taken so long to get to them. But there is always hope for the future and while we are early in the 2.15 cycle I'm going to take another pass at moving all of them forward. Cheers Arthur

@Arthur-Milchior
Copy link
Member Author

I am very thankful for your to merge it. But, and it's a HUGE but.

This never worked on travis. It have worked on computer for quite some time, but I've never been able to find a way to have it work on travis, without using an alternate CollectionTask that sends nothing to background.
This mean that if we ever goes back to travis or anything similar, the test may fail.

@Arthur-Milchior Arthur-Milchior deleted the getCardQuicker branch December 8, 2020 15:45
mikehardy added a commit that referenced this pull request Dec 8, 2020
@mikehardy
Fix null pointer in testing after #6035
8ed5cfe 
The getCard method is known to be Nullable and reset is known to be asynchronous
There is already mitigation in the method for reset waits but now it needs to handle
possible null before reset finishes as well
@mikehardy
Copy link
Member

I just fixed the reason it wasn't working ;-), check the linked commit here that just went in
I was able to get it to "reliably" fail, maybe 20% of times locally, which was enough to isolate things
After that commit I'm 100% stable

@Arthur-Milchior
Copy link
Member Author

Oh.

I always thought it was the call below to note() that failed. That makes so much sens now. Of course all my tries would fail. thanks a lot !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-allison david-allison Awaiting requested review from david-allison

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.15 release
Development

Successfully merging this pull request may close these issues.
3 participants
@Arthur-Milchior @timrae @mikehardy