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

Improve calculate_stats performance #23

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

elstob
Copy link

@elstob elstob commented Aug 19, 2024

Since recent changes to Wanikani the heatmap function calculate_stats is being ran every time a review is completed during a review session. (e.g. both reading and meaning are correct).

Unfortunately calculate_stats is quite inefficient in that it loops over every review and lesson multiple times as part of it's processing. For users with large numbers (hundreds of thousands) of reviews this can cause a lagging sensation while the numbers are crunched. This lag prevents the page updating making the act of entering reviews via the keyboard a worse experience.

A large amount of this processing time is spent casting and operating on various Date objects that are frequently used as keys for data structures in the codebase, by reducing the number of calls to Date constructors and methods like toDateString performance can be improved.

On my machine this PR reduces the time take to run calculate_stats from ~600ms to ~350ms.

This is only a partial solution to try and improve performance, in general I would suggest a more drastic set of changes to try to reduce this ~350ms time even further. This changes could include:

  • Optimising get_streaks - this function is called by calculate_stats and suffers from the same inefficiency of looping over all the data and calling toDateString hundreds of thousands of time.
  • Preventing calculate_stats from running during review time. I'm not sure that the top level do_stuff function (or any heatmap functionality in truth) actually needs to be running during review time. It seems to me that the heatmap code only needs to run on the users homepage and that capturing review data is handled by review_cache. If that's the case then I would posit that updating main to only initiate heatmap on the homepage would prevent any of these calculations running whilst the user is doing reviews. As I'm not certain heatmap doesn't need to be running during reviews however then I haven't made this change myself. Hopefully somebody with more insight can confirm.

@elstob
Copy link
Author

elstob commented Aug 19, 2024

Helps with #22

@lupomikti
Copy link
Collaborator

It seems to me that the heatmap code only needs to run on the users homepage and that capturing review data is handled by review_cache. If that's the case then I would posit that updating main to only initiate heatmap on the homepage would prevent any of these calculations running whilst the user is doing reviews.

Oh! I wish I had caught this better in the forum thread. I mentioned that Heatmap should not be running during review, only Review Cache; this is because main tests a regex for being only on the dashboard. Looking at the regex that is in the code I believe it is accidentally accepting the reviews url as valid. Can you test replacing the contents of that condition with /^https:\/\/www\.wanikani\.com(\/dashboard.*)?\/?$/.test(window.location.href) and report whether Heatmap continues to run main on reviews and lessons?

@elstob
Copy link
Author

elstob commented Aug 19, 2024

I don't have another batch for ~ 9 hours or so but I will then :)

@elstob
Copy link
Author

elstob commented Aug 19, 2024

OK so it looks like the problem isn't that the regex isn't doing the job properly in it's current state. Instead (and I think this is where the new update has caused the problem) - when you click the link from the homepage to reviews the page no longer does a full refresh but just transitions like any other single page application (e.g. React etc).

This basically means that the original instantiation of the heatmap never "unloads" now as it's still classed as the same window session. So the regex check never even happens against reviews URL as it already ran against the homepage.

Not sure if there is any easy way to hook into the page transition events now to catch this. My first instinct would be to update do_stuff to add a check for the URL the user is on and if it's not the homepage (like the current regex check) then instead of doing all the current stuff it does instead just return immediately. This should fix the issues listed above.

In an ideal world you'd tear down all the heatmap stuff and unsubscribe from review_cache when the page navigated away but I think that's out of scope.

@lupomikti
Copy link
Collaborator

Oh using the events isn't that bad, that's what I'm already doing with the event listener on turbo:load that calls main. Instead it seems like the problem is that Heatmap sets the Review Cache subscription callback to do_stuff and as Review Cache does need to run during reviews it is calling this full function over and over again.

A refactor of both Review Cache and Heatmap is probably needed to fully solve this performance issue.

But I'll see what can be done in do_stuff to mitigate things.

@elstob
Copy link
Author

elstob commented Aug 19, 2024

Yeah it should be trivial to bail out of do_stuff if not on the homepage to stop all the heatmap updating while doing reviews.

If you do that this PR isn't much more than a performance improvement to the heatmap in general so feel free to close and/or merge as you see fit either way.

@bashenk
Copy link
Contributor

bashenk commented Aug 25, 2024

Speaking of review cache and heatmap. I neglected to follow up my PR for the review cache with an update of the version referenced in Heatmap. Would be handy if you could update the @require for Review Cache to version 1428101.
🙏

@lupomikti
Copy link
Collaborator

Yes, I did notice that right after merging the change. I was torn on whether to push it right away or to wait until I've made other planned changes to Review Cache (like that import/export dialog). But in any event I will make that change in Heatmap when doing the changes related to this PR.

Speaking of, apologies for not getting to it yet. Been a bit busier than I'd like.

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.

3 participants