[pull] main from ietf-tools:main - #277
Merged
Merged
Conversation
* perf: stop the document search author and name filters cross-multiplying
Searching by author ORed five lookups into a single filter(), spanning
documentauthor -> person -> {alias,email} and rfcauthor -> person ->
{alias,email}. Because they share one filter() call, Django cannot split
them across separate joins, so the two author paths cross-multiply. The
name filter added a third multi-valued relation, targets_related, to the
same OR group.
For /doc/search/?author=Barry+Leiba&by=author&name=rfc&rfcs=on that meant
18 joins and roughly 126M intermediate rows to return 32 documents: 118
seconds, and 745M shared buffer hits. The planner estimated 7 rows for
the node that produced 41.9M, so it committed to nested loops; the
misestimate is not fixable by tuning, only by removing the fan-out. The
same search with drafts included did not complete at all, failing with
"could not resize shared memory segment ... No space left on device".
Resolve the author text to people first and match documents by primary
key, and reach related documents through a pk subquery instead of the
targets_related join. Measured 118s -> 0.14s on the same data.
Skip the draft-state clause when no draft state is allowed. On the
types-driven path that excludes nothing -- "draft" is only in `types`
when at least one draft state is allowed -- while still emitting two
joins that multiplied rows in the rest of the query. The all_types path
does not consult `types`, so there it keeps excluding drafts explicitly.
When the clause does apply, express it as a subquery too: a document has
several state rows, so joining duplicated every non-draft that passed the
other half of the OR.
That was the only thing requiring the trailing distinct(), so drop it.
It was applied to the full column list, which prepare_document_table
widens further with select_related(), forcing a sort over 72 columns
including abstract, group description and person biography -- about 1GB
of temp spill on the search above.
Verified against a full-size database: 26 query shapes produce identical
result sets with no duplicate rows, and the all_types path matches too,
including the case where no checkbox is set and drafts must be excluded.
* fix: give each document search its own cache entry
The search cache key was built from request.GET via QueryDict.items(),
which returns only the last value of a multi-valued key. doctypes is a
ModelMultipleChoiceField, so
?doctypes=charter&doctypes=statchg&name=foo
?doctypes=statchg&name=foo
hashed to the same key while returning different result sets, and each
could be served the other's results.
Derive the key from the validated cleaned_data instead. That fixes the
collision and also stops equivalent spellings of the same search from
splitting the cache -- "on" and "1" for a checkbox now agree, and the
order the values of a multi-valued field arrive in no longer matters.
'sort' stays out of the key, as before: prepare_document_table applies
the requested sort on every request.
* perf: cache document search result ids rather than prepared documents
The search view cached the fully prepared Document objects: their
prefetch caches, latest_event_cache DocEvents, milestones, sessions and
select_related Person and Group rows. A 200-row all-types search pickles
to 1.28MB, over memcached's default 1MB item limit -- which is why
LenientMemcacheCache exists to swallow "object too large for cache". The
searches most worth caching were therefore never cached, and recomputed
on every request.
Cache the matching document ids and re-prepare on a hit. The payload
becomes a few hundred integers.
This also fixes a second problem. 'sort' is not part of the cache key,
but prepare_document_table sorts in place, so a cached entry carried
whatever order it was first computed with and clicking a column header
could return the previous ordering. Re-preparing on each request applies
the requested sort every time.
Re-preparing does mean the rows arrive in whatever order the pk lookup
returns, and prepare_document_table's sort is stable, so tie-heavy sort
keys -- ipr, status and ad -- would render a page differently depending
on whether it hit the cache. Order the lookup the same way
retrieve_search_results does. That ordering is now ('-time', 'pk'):
`time` alone is not a total order, documents sharing a timestamp to the
second are common, and without the tie-break both the set of rows kept by
the max_results truncation and the order equal sort keys fall back to
vary between executions. This replaces arbitrary ordering with stable
ordering; it does not reorder anything that was previously well defined.
Use "is None" rather than a falsy test so an empty result set is a cache
hit instead of being recomputed every time.
* fix: make fill_in_telechat_date actually fill in the telechat date
The two lines that stored the result were commented out, so the function
ran a query over TelechatDocEvent, walked the results and threw them
away. Every row of the document table then called doc.telechat_date()
itself, one latest_event() query apiece -- 103 of them on a 187-row group
search -- and the five IESG views that call this before filtering on
doc.telechat_date() paid the same per-document cost.
Restore the assignment, and set the remaining documents to None so a
document with no telechat event does not fall through to the method
either. Order by ('-time', '-id') to match Document.latest_event(), so
the precomputed value equals what the method returns; a test covers that
equality, because a mismatch here silently drops documents off a telechat
agenda.
wrap_value becomes a module-level callable class rather than returning a
lambda. ietf.doc.views_search.recent_drafts pickles prepared documents
into the slowpages cache, which is file-based in production, so a lambda
would have made /doc/recent/ raise "AttributeError: Can't get local
object". Dev and test configure that cache as a DummyCache, which never
serializes, so nothing in the suite would have caught it -- hence the
accompanying picklability test.
It also defines __hash__ alongside __eq__, which Python would otherwise
set to None -- a method it stands in for is hashable.
* perf: look up review assignments once for the whole document table
fill_in_document_table_attributes called review_assignments_to_list_for_docs
with a single-element list from inside its per-document loop, even though
the helper takes a list and is built to answer for many documents at
once. Each call ran a breadth-first walk of the replaces graph -- one
query per level -- plus its own assignment query. On a 187-row group
search that was 342 queries where three suffice.
Hoist the call out of the loop and index the result by document name.
The assignment queryset only prefetched "result" while the table renders
the request's document, team and type and links to the review itself, so
each assignment shown cost several more queries. select_related them.
That also benefits the other two callers, ietf.doc.views_doc and
ietf.iesg.agenda.
* perf: batch the document relations the search table renders
Each row of the document table reads several relations: friendly_state
asks for the replacements and whether the draft became an RFC, the RFC
branch asks what subseries the document is part of, and the date column
asks what it replaces. Document.related_that and related_that_doc run a
query apiece, so those were roughly 370 queries on a 187-row search.
The IPR column was worse. Document.related_ipr collects disclosures
against the document and everything it transitively obsoletes or
replaces, and it walks that graph with all_relations_that_doc, which
issues a query per node it visits, per document -- another 244.
Fetch all of it up front: two queries for the relations the table reads,
and a breadth-first walk of the obs/replaces graph for the whole result
set at once, one query per level of depth plus one for the disclosures.
related_that and related_that_doc consult a per-instance cache when one
has been seeded. The cache is read-only and opt-in: nothing else in the
tree sets those attributes, and an unseeded document falls through to the
original query, so every other caller is unaffected.
related_ipr is attached wrapped rather than as a bare list so the
attribute stays callable like the method it shadows; templates auto-call
either way, but a bare list would make doc.related_ipr() a TypeError for
any Python caller handed a prepared document.
* perf: batch the person lookups behind the AD and shepherd columns
select_related("ad") gives every row of the document table its own Person
instance, so the memoisation on Person.email() never helps across rows.
person_link also asked, per row, whether the person's name is recorded as
one of their aliases. Between them that was 178 queries on a 187-row
search, plus more for each action holder listed.
Add Person.has_alias_for_name(), which is the expression person_link had
inline, memoised per instance and dropped in save() where the aliases
change. Seed it, and Person.email()'s own cache, for the ADs, shepherds
and action holders of a whole result set at once: the emails come from
prefetches, so only the aliases need a query, and one covers all of them.
Action holders are only read when the document has them enabled, which is
the condition the template applies too, so callers that skipped the
prefetch pay nothing extra.
person_link is used by 62 templates. Every call site passes a Person, and
plain_name() is evaluated before the changed line, so anything else would
already have failed there.
The address sort key is casefolded because Email.address is a CICharField,
so the database orders it case-insensitively and a byte-wise Python sort
could otherwise pick a different address than Person.email() would.
* perf: stop the document table refetching rows it already has
Six small fixes to fill_in_document_table_attributes and its querysets,
each independently visible in a query trace of a search page.
The latest-event cache selected every published_rfc, ballot-position,
started_iesg_process and new_revision event for the whole result set,
ordered ascending, and let later rows overwrite earlier ones in Python. A
draft routinely has dozens of new_revision events. Use DISTINCT ON to
fetch one row per (document, type) instead.
Documents with no ballot event at all had no `ballot` attribute, so
ballot_icon fell back to doc.active_ballot() -- a query apiece. Default it
to None, which is exactly what that fallback returns: same event types,
same ('-time', '-id') ordering, same created_ballot test.
The milestone sort key read m.state.slug when m.state_id already is the
slug, costing a query per milestone. The obsoleted-by and updated-by
lookup select_related only the target, then read rel.source.
select_related "type", which search_heading renders for every non-draft
row, and "shepherd__person", which the shepherd column reads. Prefetch
the action holders and their emails, which the status column reads.
Drop the "iprdocrel_set" prefetch: nothing uses it. The table renders
doc.related_ipr, which builds its own queryset, and Document.ipr() calls
.filter() on the related manager, which discards a prefetch and requeries
anyway -- ?sort=ipr measured identical at 187 per-row queries with and
without it.
* test: guard the document table against per-row queries
Rendering the table fills every attribute in for the whole result set at
once, so the number of queries should not depend on the number of rows.
Assert that directly: render a group search, triple the result set, and
require the query count not to move. A lookup that slips back into the
per-row path adds at least one query per new document, so it fails loudly
rather than quietly costing a few hundred queries a page.
Written as an invariant rather than a fixed number so it does not need
updating whenever the fixed setup cost changes.
* chore: use QuerySetAny for isinstance check
Avoids mypy lint
---------
Co-authored-by: Jennifer Richards <jennifer@staff.ietf.org>
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )