PLUTON-19231 | Optimize vcl rendering fix #516 #518
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, related domains and domain mappings for a given cluster were fetched in a loop, performing many small selects In this change, two types of of data fetching procedures (acquiring all domain mappings, and static domain mappings for each cluster) have been moved to VclRendererInput, and the smaller chunks needed for rendering steps are obtained locally from simple data structures. In local testing, this resulted in roughly halving the time of the prepare_redirects rendering phase (from around 2 to around 0.9 seconds)
mgolski
force-pushed
the
mg-ms-pluton-19231-optimize-render
branch
from
e3aec29
to
502cce2
Compare
mgolski
changed the title
awmackowiak
reviewed
Oct 18, 2024
docker-compose.dev.yml
Outdated
| @@ -112,6 +112,33 @@ services: | |||
| networks: | |||
| vaas: | |||
| ipv4_address: 192.168.199.6 | |||
| varnish-6.0.2-1: | |||
There was a problem hiding this comment.
Those extra definitions of varnishes aren't necessary.
| @@ -2,5 +2,5 @@ | |||
| DEBUG=False | |||
| TEMPLATE_DEBUG=False | |||
| DJANGO_SETTINGS_MODULE=vaas.settings.docker | |||
| PROMETHEUS_ENABLE=True | |||
| PROMETHEUS_ENABLE=False | |||
There was a problem hiding this comment.
how is this connected with optimization?
There was a problem hiding this comment.
We don't have any of the prometheus mocks, so if you run the dev server, it will try to send the metrics from celery worker about the processed task. It is easy to analyse logs in a terminal.
vaas/vaas/vcl/renderer.py
Outdated
| @collect_processing | ||
| def prepare_redirects(self) -> Dict[str, List[VclRedirect]]: | ||
| redirects = {} | ||
| related_domains = MappingProvider(DomainMapping.objects.all()).provide_related_domains(self.varnish.cluster) | ||
| related_domains = self.provide_related_domains(self.varnish.cluster) | ||
| destinations_dict = self.filter_cluster_domain_mappings(self.varnish.cluster) | ||
| for redirect in self.input.redirects: |
There was a problem hiding this comment.
The better way will be an iteration on the prepared redirects for related domains.
I have prepared a draft PR how it can look like:
#519
mgolski
force-pushed
the
mg-ms-pluton-19231-optimize-render
branch
from
f41ec46
to
5b5a967
Compare
Draft pull request: #519 This changes the iteration order during prepare_redirects (changing the loop from: ```for redirect in self.input.redirects: ... for mapped_domain in redirect.src_domain.mapped_domains(self.varnish.cluster): ``` to: ```for related_domain in related_domains: ... for redirect in self.input.redirects.get(related_domain): ... for mapped_domain in redirect.src_domain.mapped_domains(self.varnish.cluster): ``` In testing this caused a speedup in the time to prepare redirects from ~0.7 to ~0.1
mgolski
force-pushed
the
mg-ms-pluton-19231-optimize-render
branch
from
5b5a967
to
1dd072b
Compare
ziollek
changed the title
ziollek
approved these changes
Oct 21, 2024
awmackowiak
reviewed
Oct 22, 2024
vaas/vaas/resources/dev-data.yaml
Outdated
| condition: req.url ~ "/source_url" | ||
| destination: http://mydomain.com/destination_url | ||
| action: 301 | ||
| priority: 250 | ||
| preserve_query_params: false | ||
| - model: router.redirect |
There was a problem hiding this comment.
To be sure, should we add this amount of the same redirects for the development environment?
There was a problem hiding this comment.
Removed the extra redirects in 9630c5a
awmackowiak
approved these changes
Oct 23, 2024
ziollek
approved these changes
Oct 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, related domains and domain mappings for a given cluster were fetched in a loop, performing many small selects.
In this change, two types of of data fetching procedures (acquiring all domain mappings, and static domain mappings for each cluster) have been moved to
VclRendererInput, and the smaller chunks needed for rendering steps are obtained locally from simple data structures.In local testing, this resulted in roughly halving the time of the prepare_redirects rendering phase (from around 2 to around 0.9 seconds)