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

Heatmap speed improvements #4755

Merged
merged 3 commits into from
Dec 6, 2023
Merged

Heatmap speed improvements #4755

merged 3 commits into from
Dec 6, 2023

Conversation

dracos
Copy link
Member

@dracos dracos commented Dec 6, 2023

The state change should help everywhere, but it's most noticeable on the heatmap [skip changelog]

@dracos dracos requested a review from davea December 6, 2023 10:57
@dracos dracos force-pushed the heatmap-speed-improvements branch from 11dd75c to 91ba974 Compare December 6, 2023 11:08
Copy link

codecov bot commented Dec 6, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (51ace92) 85.30% compared to head (91ba974) 85.40%.

❗ Current head 91ba974 differs from pull request most recent head 77a2ea8. Consider uploading reports for the commit 77a2ea8 to get more accurate results

Files Patch % Lines
perllib/FixMyStreet/Cobrand/CheshireEast.pm 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4755      +/-   ##
==========================================
+ Coverage   85.30%   85.40%   +0.09%     
==========================================
  Files         339      339              
  Lines       24585    24793     +208     
  Branches     4666     4751      +85     
==========================================
+ Hits        20973    21174     +201     
+ Misses       2191     2189       -2     
- Partials     1421     1430       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@davea davea left a comment

Choose a reason for hiding this comment

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

Woo! This is much much faster now ⚡

On reports pages (and heatmap), we know all reports involved are owned
by the body, so do not need to perform this check. Copy of e4addf2
for other cobrands.
The prefetch used in the sidebar queries is
not needed when fetching the pin data.
Even though these are saved in memcache, if we need to look it up a lot
of times in one request, is quicker to cache it locally as well.
@dracos dracos force-pushed the heatmap-speed-improvements branch from 91ba974 to 77a2ea8 Compare December 6, 2023 15:55
@dracos dracos merged commit 77a2ea8 into master Dec 6, 2023
19 checks passed
@dracos
Copy link
Member Author

dracos commented Dec 6, 2023

After testing this live, I got annoyed by the initial page loading speed (the pins after that were much better from the above), given it was then reloading the same data, and saw some double loading, so I added another commit, 660f74d which stopped the sidebar from being included at initial load, and made sure the markers were hidden to prevent a double load of (admittedly now faster) data.

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.

2 participants