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

Feat/support load balance and easy days in rescheduling #3815

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

L-M-Sherlock
Copy link
Contributor

@L-M-Sherlock L-M-Sherlock commented Feb 13, 2025

It supports load balance and easy days natively in rescheduling. For the sake of performance, I didn't reuse the LoadBalancer and LoadBalancerContext.

Preview:

25.02:

image

The PR:

image

The PR with easy days:

imageimage

@jakeprobst
Copy link
Contributor

jakeprobst commented Feb 13, 2025

For the sake of performance, I didn't reuse the LoadBalancer and LoadBalancerContext.

this feels like a setup for future sadness, how bad was the perf hit that made this the preferable choice?

At the very least, I'd say move everything that is common to both into their own functions so the logic will be consistent across them both. this will also make it easier to see the ways in which these differ, like I can tell they are sliiightly different but its hard to determine where without also pulling up the other code, if all common logic is in the same spot it'll be clear what the differences are.

@L-M-Sherlock
Copy link
Contributor Author

this feels like a setup for future sadness, how bad was the perf hit that made this the preferable choice?

This function is I/O bound:

impl LoadBalancer {
pub fn new(
today: u32,
did_to_dcid: HashMap<DeckId, DeckConfigId>,
next_day_at: TimestampSecs,
storage: &SqliteStorage,
) -> Result<LoadBalancer> {
let cards_on_each_day =
storage.get_all_cards_due_in_range(today, today + LOAD_BALANCE_DAYS as u32)?;

For rescheduling, the today is the last review date, so I have to re-create load balancer for each card. It's very slow. It would take 1 minute to reschedule thousands of cards.

@dae
Copy link
Member

dae commented Feb 13, 2025

For future maintenance, we definitely want as much code shared between the paths as possible - whether that's by reusing old code and doing things like mutating an existing context for each card instead of creating a new one (which may not be practical here), or by introducing this new code and having the old code use it.

@YukiNagat0
Copy link
Contributor

Does this change respect the mw.col.load_balancer_enabled option?

@L-M-Sherlock
Copy link
Contributor Author

Does this change respect the mw.col.load_balancer_enabled option?

It does now.

For future maintenance, we definitely want as much code shared between the paths as possible - whether that's by reusing old code and doing things like mutating an existing context for each card instead of creating a new one (which may not be practical here), or by introducing this new code and having the old code use it.

I'm reusing some code snippet now.

@L-M-Sherlock L-M-Sherlock marked this pull request as ready for review February 13, 2025 07:07
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.

4 participants