-
Notifications
You must be signed in to change notification settings - Fork 8
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
base: main
Are you sure you want to change the base?
Conversation
Helps with #22 |
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 |
I don't have another batch for ~ 9 hours or so but I will then :) |
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 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. |
Oh using the events isn't that bad, that's what I'm already doing with the event listener on 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 |
Yeah it should be trivial to bail out of 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. |
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 |
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. |
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:
get_streaks
- this function is called bycalculate_stats
and suffers from the same inefficiency of looping over all the data and callingtoDateString
hundreds of thousands of time.calculate_stats
from running during review time. I'm not sure that the top leveldo_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 byreview_cache
. If that's the case then I would posit that updatingmain
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.