Improve performance of query materializer - #470
Merged
Merged
Conversation
prefetch_related() issues its own query built from Server._base_manager,
so defer("server__intern_ip") on the outer queryset never reached it -
that spelling only takes effect for select_related() traversals. Every
prefetched Server therefore parsed intern_ip through netfields and
ipaddress, which is pure Python and accounted for ~10% query time of
profiled queries, even though intern_ip is not a real attribute and never
appears in a restrict clause.
Pass an explicit Prefetch queryset at the two sites where the prefetched
Server is only used as a dict key. The reverse branch keeps the full row
on purpose: sa.server is stored as an attribute value there, so it can
escape into the results and reach _get_servers_to_join(), which reads
intern_ip - deferring it would cost a query per object.
This can be reverted once we removed intern_ip fully.
_add_supernet_attribute() read its join through Server.objects.raw(), which builds one model instance per result row. The join yields a row per (host, net) pair, so a large query created tens of thousands of Server objects - each parsing net.intern_ip through netfields - to represent far fewer distinct networks. Select only the ids, read them through a plain cursor, and fetch the distinct nets once. This trades two extra round trips for fewer model instantiations and the same number of inet parses which pays out especially on queries with many results. The nets are still fetched whole: they are stored as attribute values and can reach a nested QueryMaterializer via _get_servers_to_join(), which reads intern_ip. host_attr_name used to ride along on each per-row Server object and was read back off the previously stored supernet for the conflict warning. One net object is now shared by every host inside it, so it is tracked separately, keyed by host id. The two inet_address_family columns the query selected were never read by anything and are gone.
_server_attributes was keyed by Server instances. Model.__hash__() is a Python-level call that goes through _is_pk_set() and the pk property, and the map is looked up once per server per attribute - 1174077 times on a large query. Plain ints hash in C. The key change pays for itself twice over: with an int key the generic branch of _add_attributes() can take sa.server_id straight off the attribute row, so its prefetch_related() of Server disappears - and with it the huge IN ((1),(2),...) lists Django was compiling for those prefetches, which alone were about a fifth of the request. That branch covers string, number, boolean, inet, macaddr, date and datetime, so it is the bulk of the rows. The Server objects it used to fetch were only ever dict keys. The reverse branch now keys off sa.value_id instead of resolving the FK, and _add_supernet_attribute() uses the host_server_id its own SQL already returns. In a query with hundreds and thousands of results this way the function calls can be halved and runtime could be reduced by circa 40%. Three places still key by Server on purpose: servers_by_related in _add_related_attribute(), whose keys feed a hostname filter; _get_join_results(); and the reverse branch, which stores sa.server as an attribute value rather than using it as a key. All three are small next to the main map.
The query materializer keys its per-server attribute dicts by Attribute instances, so every lookup goes through Model.__hash__(). That is a Python-level method which first calls _is_pk_set() and then reads the pk property before hashing - three calls for what is, for this model, just hash(self.attribute_id), since attribute_id is the primary key. Those dicts are hit once per server per attribute: Hundreds of thousands of times on a queries with thousand objects. The override returns the identical value and skips the two extra calls. Equality stays Django's, so hash and eq remain consistent. The only behavioral difference is that an unsaved Attribute without an attribute_id becomes hashable, which Django forbids on purpose. Here attribute_id is a natural key assigned before save, so the case does not arise. I currently think this is safe to do but if it makes any problems feel free to revert this commit is isolated from the other changes.
The generic branch of QueryMaterializer._add_attributes() built a model instance for every attribute row - over a hundred thousand on a large query - only to read server_id, attribute_id and value once each. An instance costs Model.__init__() plus the pre_init and post_init signals; that machinery was about 15% runtime on profiled requests. Read the rows with values_list() instead. The per-type conversion get_value() applied is now also available as a classmethod, convert_db_value(), so the knowledge stays on the models: number keeps its Decimal-to-int/float rule there and get_value() delegates to it, boolean returns True as before since its table has no value column, and relation raises NotImplementedError because its column is a server_id that cannot become a Server without a query - the materializer resolves those targets with one bulk query instead of a prefetch per chunk. The branch moves into its own method, _add_stored_attributes(), since the two type checks would otherwise push _add_attributes() past the project's cyclomatic complexity limit. It clears the relation manager's default prefetch of "value", which would run against the tuples and fail, and is redundant given the bulk fetch anyway. Inet values still pass through the netfields converter; values_list() applies field converters like the model path does. I was unsure whatever it is worth to make this change the LLM suggested but found the way with the classmethod acceptable in readability vs performance trade-off.
Contributor
Author
|
This code has been written with the assistance of LLMs but manually reviewed and tested. I find the changes meaningful and understandable. Two commits are questionable in thus as they make changes to the standard way Django works and can be dropped if they cause any problems. |
brainexe
approved these changes
Sep 23, 2026
brainexe
left a comment
brainexe
left a comment
Member
There was a problem hiding this comment.
manual tests passed, code looks valid ✔️
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR contains changes reducing the runtime of queries returning thousands of objects.