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

Improved page loading #546

Merged
merged 5 commits into from
Jul 13, 2023
Merged

Improved page loading #546

merged 5 commits into from
Jul 13, 2023

Conversation

jsharkey13
Copy link
Member

None of these methods used question attempts, and locally there is not much difference between the speed of the old method and the new one. Sometimes it could be 75% faster, sometimes the same time; it was never worse which is good enough for me. I expect better results on the server (where the cache is helpful and other things are happening in the API).

We now consistently used getContentById for loading pages and fragments of all kinds, and so findSingleResult can and has been removed. The code is easier to read since all the methods are very similar to one another. A later improvement might be to reduce the code duplication.

Unlike findSingleResult, the getContentById method uses the cache in
GitContentManager. This should speed up loading commonly-accessed pages.
Same as 33e71c1 but for general pages.
Same as 33e71c1 but for page fragments.
We now consistently use the cache-based getContentById. The only other
difference (beyond this old method lacking caching) is that this errored
out if more than one piece of content matched, but I think that's
no big loss.
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (96c6c84) 27.70% compared to head (fbd3012) 27.69%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #546      +/-   ##
==========================================
- Coverage   27.70%   27.69%   -0.01%     
==========================================
  Files         504      504              
  Lines       22931    22933       +2     
  Branches     2837     2835       -2     
==========================================
  Hits         6352     6352              
- Misses      15979    15981       +2     
  Partials      600      600              
Impacted Files Coverage Δ
...ain/java/uk/ac/cam/cl/dtg/isaac/api/Constants.java 77.27% <ø> (ø)
...n/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java 19.18% <0.00%> (-0.17%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

We removed the type this corresponded to in
72cb910
Copy link
Contributor

@mlt47 mlt47 left a comment

Choose a reason for hiding this comment

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

Looks good. Shared DO cache between users should hopefully make a visible difference to response times for these endpoints.

@mlt47 mlt47 merged commit 097aed9 into master Jul 13, 2023
3 checks passed
@mlt47 mlt47 deleted the feature/improved-page-loading branch July 13, 2023 14:55
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