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

Optimize resolve_path #6867

Merged
merged 11 commits into from Apr 13, 2020
Merged

Optimize resolve_path #6867

merged 11 commits into from Apr 13, 2020

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Apr 6, 2020

This reduces: 2 queries for "normal" projects; and 4 for "complex" projects.

There are only 2 tests falling

def test_resolver_no_force_translation(self):
with override_settings(USE_SUBDOMAIN=False):
url = resolve_path(
project=self.translation, filename='index.html', language='cz',
)
self.assertEqual(url, '/docs/pip/ja/latest/index.html')
with override_settings(USE_SUBDOMAIN=True):
url = resolve_path(
project=self.translation, filename='index.html', language='cz',
)
self.assertEqual(url, '/ja/latest/index.html')
def test_resolver_no_force_translation_with_version(self):
with override_settings(USE_SUBDOMAIN=False):
url = resolve_path(
project=self.translation, filename='index.html', language='cz',
version_slug='foo',
)
self.assertEqual(url, '/docs/pip/ja/foo/index.html')
with override_settings(USE_SUBDOMAIN=True):
url = resolve_path(
project=self.translation, filename='index.html', language='cz',
version_slug='foo',
)
self.assertEqual(url, '/ja/foo/index.html')

But can't find a place where we call the resolver with a wrong language, we usually do

return self.project.get_docs_url(
version_slug=self.slug,
lang_slug=self.project.language,
private=private,
external=external,
)

Notes:

  • One extra query is still done for custom installations not using a subdomain
  • One 1 test pass with the old and new code, I think the other one we need to agree with what the "canonical" project is. We decided to not support subprojects of translations for now.

ref #6455

cname = (
cname
or self._use_subdomain()
or main_project.get_canonical_custom_domain()
)
Copy link
Member Author

@stsewd stsewd Apr 6, 2020

Choose a reason for hiding this comment

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

I thought we were making use of the real cname, but we only use it as a boolean value to see if we need to serve from /docs instead of /.

@stsewd stsewd added the PR: work in progress label Apr 7, 2020
@stsewd stsewd removed the PR: work in progress label Apr 7, 2020
subproject_slug = relation.alias

return (main_project, subproject_slug)

def _get_canonical_project(self, project, projects=None):
Copy link
Member Author

@stsewd stsewd Apr 7, 2020

Choose a reason for hiding this comment

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

we can re-use the above method here if we don't't want to support more than 2 levels of relationships (thing that we already don't support actually...)

Copy link
Member

@ericholscher ericholscher Apr 7, 2020

Choose a reason for hiding this comment

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

I don't understand this comment? re-use what method?

Copy link
Member Author

@stsewd stsewd Apr 7, 2020

Choose a reason for hiding this comment

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

This method is basically the same as _get_canonical_project_data (when checking till 2 levels of relationships)

stsewd added a commit that referenced this issue Apr 7, 2020
This together with:
- #6867
- #6865
- #6864

reduces a lot of queries and without changing the current UI.

This is a benchmark using the django debug toolbar:

Before: 138 queries with 7 subprojects, 17 queries per each additional subproject

With all the changes: 28 queries with 7 subprojects, 1 query per each additional subproject
@stsewd stsewd requested a review from Apr 7, 2020
Copy link
Member

@ericholscher ericholscher left a comment

This looks good at first, but as with all resolver changes, it scares me a bit. A few small items.

readthedocs/core/resolver.py Outdated Show resolved Hide resolved
readthedocs/core/resolver.py Outdated Show resolved Hide resolved
subproject_slug = relation.alias

return (main_project, subproject_slug)

def _get_canonical_project(self, project, projects=None):
Copy link
Member

@ericholscher ericholscher Apr 7, 2020

Choose a reason for hiding this comment

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

I don't understand this comment? re-use what method?

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 7, 2020

but as with all resolver changes, it scares me a bit

I can split this PR into 3 PRs if that makes it more easy

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Apr 9, 2020

I can split this PR into 3 PRs if that makes it more easy

I think it's just scary in general, more PRs wont make it less scary :)

Copy link
Member

@ericholscher ericholscher left a comment

This looks good, but I'd like to write up some QA steps in the release project before we merge it, if you have some ideas for that.

@@ -204,6 +188,68 @@ def resolve(
)
return urlunparse((protocol, domain, path, '', query_params, ''))

def _get_canonical_project_data(self, project):
"""
Returns a tuple with (project, subproject_slug) from the canonical project of `project`.
Copy link
Member

@humitos humitos Apr 13, 2020

Choose a reason for hiding this comment

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

💯 docstring!

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 13, 2020

I have added some projects to check on the release card

@stsewd stsewd merged commit ecba0f1 into master Apr 13, 2020
2 checks passed
@stsewd stsewd deleted the optimize-resolve-path branch Apr 13, 2020
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