Opened 3 years ago

Closed 3 years ago

#33124 closed Cleanup/optimization (fixed)

Avoid accessing ConnectionsHandler.__getitem__ until it's strictly necessary.

Reported by: Keryn Knight Owned by: Keryn Knight
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Follows on from #33025.

Most of the codebase does the correct thing as far as I can see, preferring conn(ection) = connections[alias] and holding a reference to that, when it's used multiple times. Most of the codebase also has touching connections[alias] as the last component in if branches etc too, so that short circuiting can occur to avoid touching the Local.

There's a few places where that isn't the case though. eg:

empty_strings_as_null = connections[db].features.interprets_empty_strings_as_nulls
...
if val is None or (val == '' and empty_strings_as_null):

or

can_ignore_conflicts = (
    connections[db].features.supports_ignore_conflicts and
    self.through._meta.auto_created is not False
)

or

compiler = connections[db].ops.compiler('SQLCompiler')(
    self.query, connections[db], db
)

Whilst I've not gone to the effort of benchmarking each of them, each of them has the possibility to slightly improve things, I think.

I'll be pushing a PR shortly with a bunch of commits that cover all of those I've found (outside of tests, where things are more gnarly and could be tackled separately, if there's an impetus to do so), so that CI can prove things work, and so that it can be decided which, if any of them, are worth it.

Change History (5)

comment:1 by Keryn Knight, 3 years ago

Has patch: set
Patch needs improvement: set

PR is https://github.com/django/django/pull/14876
Marking as patch needs improvement because it's a whole bunch of commits, none with a 'proper' commit message, and also because we may not wish to pull _all_ of the proposed changes in.

comment:2 by Mariusz Felisiak, 3 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Mariusz Felisiak, 3 years ago

Summary: Avoid accessing ConnectionsHandler.__getitem__ in a few more places until it's strictly necessary.Avoid accessing ConnectionsHandler.__getitem__ until it's strictly necessary.

comment:4 by Mariusz Felisiak, 3 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:5 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 06c50cee:

Fixed #33124 -- Avoided accessing the database connections when not necessary.

Follow up to bf5abf1bdcedb15e949db419c61eeec7c88414ea.

This also caches the getitem access.

Note: See TracTickets for help on using tickets.
Back to Top