From 50961a9196ea5d25525009e7b69d9be397fced6c Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Tue, 1 Jun 2021 14:50:35 +1000 Subject: [PATCH] push review randomizing into SQL This makes the review backlog case more expensive, since we end up shuffling items outside the daily limit, but for the common case it's about the same speed, and it means we don't need two separate sorting steps. New cards remain handled the same way, since a backlog is common there. Also ensures that interday learning cards honor the deck sorting, and that the non-default sort orders shuffle at the end. --- rslib/src/scheduler/queue/builder/mod.rs | 1 - rslib/src/scheduler/queue/builder/sorting.rs | 33 +------------ rslib/src/storage/card/mod.rs | 50 +++++++++++++++----- rslib/src/storage/sqlite.rs | 14 +++++- 4 files changed, 53 insertions(+), 45 deletions(-) diff --git a/rslib/src/scheduler/queue/builder/mod.rs b/rslib/src/scheduler/queue/builder/mod.rs index fbd546a8c..629194133 100644 --- a/rslib/src/scheduler/queue/builder/mod.rs +++ b/rslib/src/scheduler/queue/builder/mod.rs @@ -116,7 +116,6 @@ impl QueueBuilder { current_day: u32, ) -> CardQueues { self.sort_new(); - self.sort_reviews(current_day); // intraday learning let learning = sort_learning(self.learning); diff --git a/rslib/src/scheduler/queue/builder/sorting.rs b/rslib/src/scheduler/queue/builder/sorting.rs index 89dc678e5..549e27869 100644 --- a/rslib/src/scheduler/queue/builder/sorting.rs +++ b/rslib/src/scheduler/queue/builder/sorting.rs @@ -5,7 +5,7 @@ use std::{cmp::Ordering, hash::Hasher}; use fnv::FnvHasher; -use super::{DueCard, NewCard, NewCardSortOrder, QueueBuilder, ReviewCardOrder}; +use super::{NewCard, NewCardSortOrder, QueueBuilder}; impl QueueBuilder { pub(super) fn sort_new(&mut self) { @@ -24,19 +24,6 @@ impl QueueBuilder { } } } - - pub(super) fn sort_reviews(&mut self, _current_day: u32) { - self.day_learning - .iter_mut() - .for_each(DueCard::hash_id_and_mtime); - self.day_learning.sort_unstable_by(day_then_hash); - - // other sorting is done in SQL - if self.sort_options.review_order == ReviewCardOrder::Day { - self.review.iter_mut().for_each(DueCard::hash_id_and_mtime); - self.review.sort_unstable_by(day_then_hash); - } - } } fn template_then_due(a: &NewCard, b: &NewCard) -> Ordering { @@ -55,28 +42,10 @@ fn new_hash(a: &NewCard, b: &NewCard) -> Ordering { a.hash.cmp(&b.hash) } -fn day_then_hash(a: &DueCard, b: &DueCard) -> Ordering { - (a.due, a.hash).cmp(&(b.due, b.hash)) -} - // We sort based on a hash so that if the queue is rebuilt, remaining // cards come back in the same approximate order (mixing + due learning cards // may still result in a different card) -impl DueCard { - fn hash_id_and_mtime(&mut self) { - let mut hasher = FnvHasher::default(); - hasher.write_i64(self.id.0); - hasher.write_i64(self.mtime.0); - self.hash = hasher.finish(); - } - - #[allow(dead_code)] - fn set_hash_to_relative_overdue(&mut self, _current_day: u32) { - todo!() - } -} - impl NewCard { fn hash_id_and_mtime(&mut self) { let mut hasher = FnvHasher::default(); diff --git a/rslib/src/storage/card/mod.rs b/rslib/src/storage/card/mod.rs index 45e87ec2a..fbb4bacfa 100644 --- a/rslib/src/storage/card/mod.rs +++ b/rslib/src/storage/card/mod.rs @@ -199,17 +199,7 @@ impl super::SqliteStorage { where F: FnMut(CardQueue, DueCard) -> bool, { - let order_clause = match order { - ReviewCardOrder::Day => "due", - ReviewCardOrder::DayThenDeck => { - "due, (select rowid from active_decks ad where ad.id = did)" - } - ReviewCardOrder::DeckThenDay => { - "(select rowid from active_decks ad where ad.id = did), due" - } - ReviewCardOrder::IntervalsAscending => "ivl asc", - ReviewCardOrder::IntervalsDescending => "ivl desc", - }; + let order_clause = review_order_sql(order); let mut stmt = self.db.prepare_cached(&format!( "{} order by {}", include_str!("due_cards.sql"), @@ -552,6 +542,44 @@ impl super::SqliteStorage { } } +#[derive(Clone, Copy)] +enum ReviewOrderSubclause { + Day, + Deck, + Random, + IntervalsAscending, + IntervalsDescending, +} + +impl ReviewOrderSubclause { + fn to_str(self) -> &'static str { + match self { + ReviewOrderSubclause::Day => "due", + ReviewOrderSubclause::Deck => "(select rowid from active_decks ad where ad.id = did)", + ReviewOrderSubclause::Random => "fnvhash(id, mod)", + ReviewOrderSubclause::IntervalsAscending => "ivl asc", + ReviewOrderSubclause::IntervalsDescending => "ivl desc", + } + } +} + +fn review_order_sql(order: ReviewCardOrder) -> String { + let mut subclauses = match order { + ReviewCardOrder::Day => vec![ReviewOrderSubclause::Day], + ReviewCardOrder::DayThenDeck => vec![ReviewOrderSubclause::Day, ReviewOrderSubclause::Deck], + ReviewCardOrder::DeckThenDay => vec![ReviewOrderSubclause::Deck, ReviewOrderSubclause::Day], + ReviewCardOrder::IntervalsAscending => vec![ReviewOrderSubclause::IntervalsAscending], + ReviewCardOrder::IntervalsDescending => vec![ReviewOrderSubclause::IntervalsDescending], + }; + subclauses.push(ReviewOrderSubclause::Random); + + let v: Vec<_> = subclauses + .into_iter() + .map(ReviewOrderSubclause::to_str) + .collect(); + v.join(", ") +} + #[cfg(test)] mod test { use std::path::Path; diff --git a/rslib/src/storage/sqlite.rs b/rslib/src/storage/sqlite.rs index 22418b22f..9d2101a07 100644 --- a/rslib/src/storage/sqlite.rs +++ b/rslib/src/storage/sqlite.rs @@ -1,8 +1,9 @@ // Copyright: Ankitects Pty Ltd and contributors // License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html -use std::{borrow::Cow, cmp::Ordering, path::Path, sync::Arc}; +use std::{borrow::Cow, cmp::Ordering, hash::Hasher, path::Path, sync::Arc}; +use fnv::FnvHasher; use regex::Regex; use rusqlite::{functions::FunctionFlags, params, Connection, NO_PARAMS}; use unicase::UniCase; @@ -51,6 +52,7 @@ fn open_or_create_collection_db(path: &Path) -> Result { add_field_index_function(&db)?; add_regexp_function(&db)?; add_without_combining_function(&db)?; + add_fnvhash_function(&db)?; db.create_collation("unicase", unicase_compare)?; @@ -88,6 +90,16 @@ fn add_without_combining_function(db: &Connection) -> rusqlite::Result<()> { ) } +fn add_fnvhash_function(db: &Connection) -> rusqlite::Result<()> { + db.create_scalar_function("fnvhash", -1, FunctionFlags::SQLITE_DETERMINISTIC, |ctx| { + let mut hasher = FnvHasher::default(); + for idx in 0..ctx.len() { + hasher.write_i64(ctx.get(idx)?); + } + Ok(hasher.finish() as i64) + }) +} + /// Adds sql function regexp(regex, string) -> is_match /// Taken from the rusqlite docs type BoxError = Box;