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

Table Performance Improvements #228

Merged
merged 6 commits into from
May 16, 2020
Merged

Conversation

araujoarthur0
Copy link
Collaborator

@araujoarthur0 araujoarthur0 commented May 15, 2020

Context / Background

  • Briefly explain the purpose of the PR by providing a summary of the context

I'm improving the performance of the table when switching between months. On the last release, it takes around 1s to switch up every month. After these optimizations, it now takes around 40ms.

Memory consumption analysis with "Heap Snapshots" taken with Chrome Dev Tools showed use went from around 4MB to 6MB on an idle screen. However, on the last release the heap usage increases by 4MB when switching up pages due to re-reading from store, and on this version, it increases at most 1MB. Anyway, these memory values are so low for today's computers that I think this is negligible.

What change is being introduced by this PR?

  • How did you approach this problem? What changes did you make to achieve the goal?

This is a series of commits.

  1. First change makes the table headers static and succeeding month changes to only reload the table body and month on the header.
  2. Second change lets the preference contents be reused, avoiding that the algorithms read up the preferences from the files on every API call to functions on user-preferences.js.
  3. Third change loads up the entire electron store for the calendar in memory, avoiding .has and .get calls that would take precious milliseconds due to the nature of the storage.
  4. Fourth change is similar, but on the Waiver store instead, also adding a waiver saved event to reload waiver changes.
  5. Removing unneeded off('click').on('click') on initCalendar now that these don't get removed on each Calendar draw.
  • What are the indirect and direct consequences of the change?

It's going to feel much better for the user to use the program and look back into previous months, improving user experience.

How will this be tested?

  • How will you verify whether your changes worked as expected once merged?

Ran tests and tested with my user experience flows.

Before:
slow

After:
fast

@codecov
Copy link

codecov bot commented May 15, 2020

Codecov Report

Merging #228 into master will decrease coverage by 0.25%.
The diff coverage is 40.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
- Coverage   35.70%   35.45%   -0.26%     
==========================================
  Files          11       11              
  Lines         591      598       +7     
  Branches      101      104       +3     
==========================================
+ Hits          211      212       +1     
- Misses        337      343       +6     
  Partials       43       43              
Impacted Files Coverage Δ
js/calendar.js 0.00% <0.00%> (ø)
js/date-aux.js 62.50% <0.00%> (-37.50%) ⬇️
main.js 0.00% <0.00%> (ø)
js/time-math.js 100.00% <100.00%> (ø)
js/user-preferences.js 90.36% <100.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fec05c...a42197e. Read the comment docs.

@araujoarthur0 araujoarthur0 marked this pull request as draft May 15, 2020 02:17
@araujoarthur0
Copy link
Collaborator Author

I think I'll do some memory profiling before committing to the 1 year limit.
Does anyone have a prefilled 2 year storage for a fuller store?

Copy link
Collaborator

@tupaschoal tupaschoal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the third commit, for a future PR, how expensive would it be to store all of the data we have? Maybe with a proper callback that just updates the current month or day, we can have everything saved which would make #196 a lot easier (?)

js/classes/Calendar.js Outdated Show resolved Hide resolved
@araujoarthur0 araujoarthur0 marked this pull request as ready for review May 15, 2020 20:01
@araujoarthur0
Copy link
Collaborator Author

In case this is useful for anyone, this is a 3 year store with every date filled.

test.zip

@araujoarthur0
Copy link
Collaborator Author

I made the profilings but saw negligible memory changes.
I did however update the pull request functionally. Instead of relying on for loops to call .get on the store, I'm accessing directly the store element and saving everything to internal memory. This is also true for the waiver store.
This is a much faster loading of the store (it hanged a while with the 3 year filled store and the previous solution), and it also avoids the reloading on the change of years.
Every possible month change is now faster, no matter the year :)

@tupaschoal
Copy link
Collaborator

Sounds great, I'll test it later today and give you feedback

@thamara
Copy link
Owner

thamara commented May 15, 2020

This looks great. I think with the internalStorage solution we are just a few lines from being able to have the overall balance. I'll do the review later today.

js/calendar.js Outdated Show resolved Hide resolved
main.js Outdated Show resolved Hide resolved
Copy link
Owner

@thamara thamara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome!

@thamara thamara merged commit 68c71dc into thamara:master May 16, 2020
@thamara thamara added this to the v1.5.2 milestone May 16, 2020
@araujoarthur0 araujoarthur0 deleted the table-update branch June 19, 2020 01:38
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.

None yet

3 participants