Cached commits [RUN ALL RSPEC] [RUN AS-IF-FOSS]
What does this MR do?
Caches the _commit
partial being rendered by the _commits
partial. This uses https://gitlab.com/robotmay/chunky_cache.
There are two aspects to the commit partial which are very cache-antagonistic:
- The
time_ago_with_tooltip
call, which displays the date as e.g.16 minutes ago
rather than the actual time. - The pipeline status which is scoped to each user and updates separately from the commit
Rather than wrapping all of this state into the cache keys or expiring every minute, this MR introduces chunked caching with Redis multi-fetching, allowing us to use multiple small cache segments but only expend one network request. This also allows us to exclude those awkward elements from the cached data. In this MR, the first of those two "gotchas" above are not cached at all, and the second is cached based on what it will ultimately render.
I've introduced this as a new partial which will replace the original _commit
partial once the feature flag is removed.
This has a direct impact on two current performance issues:
Related #209912
Related #211709
chunky_cache
This is a gem I wrote recently which implements new view helpers to perform multi-fetch caching inside a view. If this MR merges I will of course add additional GitLab maintainers onto the gem project, as suggested in the Handbook.
Rollout issue)
Feature flag (:cached_commit_collection
Benchmarks
Timings from rendering two commits in an MR on the Projects::MergeRequests#commits.json
endpoint:
Original
Rendered projects/commits/_commits.html.haml (Duration: 35.9ms | Allocations: 35477)
Rendered projects/merge_requests/_commits.html.haml (Duration: 45.0ms | Allocations: 44127)
...
Rendered collection of projects/commits/_commit.html.haml [40 times] (Duration: 89.4ms | Allocations: 85294)
Cached
Rendered projects/commits/_commits.html.haml (Duration: 30.8ms | Allocations: 51203)
Rendered projects/merge_requests/_commits.html.haml (Duration: 20.8ms | Allocations: 23404)
...
Rendered collection of projects/commits/_cached_commit.html.haml [40 times] (Duration: 22.3ms | Allocations: 34401)
I expect this to scale well. It now uses only a single Redis call across all those partial renders. There will still be a cost on the Redis side for the number of keys being read, and this will be something I measure when testing it with real traffic.
Screenshots (strongly suggested)
Does this MR meet the acceptance criteria?
Conformity
-
📋 Does this MR need a changelog?-
I have included a changelog entry. -
I have not included a changelog entry because this is behind a feature flag.
-
-
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team