Opened 10 years ago

Closed 9 years ago

#3453 closed (fixed)

resolve_variable usage leads to redundant work

Reported by: (removed) Owned by: Jacob
Component: Template system Version: master
Severity: Keywords: performance
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Yet more refactoring ;)

resolve_variable does a chunk of setup work for each invocation, checking for if the variable is a number, checking if its quoted (thus exempted from lookup rules), and finally splitting of the variable into it's component parts. The issue is that this work is utterly redundant the second time an attempt is made to resolve the variable- for loops for example.

This patch splits out a Variable class which does the common setup work up once in init, with a resolve method to do the actual resolution against a context. The actual resolve is still a good chunk of time, but for my particular usage scenario (3 level deep for loop being the main cost), this shaves just shy of 25% of the runtime off via killing the redundant work.

From a backwards compatibility standpoint, there isn't any issue- resolve_variable just becomes

def resolve_variable(arg, context):
  return Variable(arg).resolve(context)

which admittedly will be slightly slower then whats in place now for simple lookups- this is offset by the greater efficiency of Variable usage within default tags/templates, and via killing off a quadratic op in resolve_variable- currently, it splits the arg, then does poplefts on the list.

Problem there is that python lists are literal arrays; you delete the first item, every item after has to be moved one to the left, thus quad in execution. Fortunately var depth isn't usually deep enough for it to rear its head.

Attachments (5)

patch (16.5 KB) - added by (removed) 10 years ago.
resolve_variable refactoring
resolve-variable.patch (20.9 KB) - added by (removed) 9 years ago.
updated version since previous no longer applies; depends on var lookup needs, but seeing around 41% drop in simple var resolution (single to double lookup).
resolve-variable.2.patch (15.4 KB) - added by (removed) 9 years ago.
Round two, since trac doesn't seem to like bzr bundles and since some extra crap slipped into the patch
resolve_variable-correct-bitrot.patch (19.5 KB) - added by (removed) 9 years ago.
correct bitrot (again)
resolve-variable-v3.diff (22.2 KB) - added by Jacob 9 years ago.
An updated and somewhat improved version of the patch in which Variable becomes an actual class.

Download all attachments as: .zip

Change History (15)

Changed 10 years ago by (removed)

Attachment: patch added

resolve_variable refactoring

comment:1 Changed 10 years ago by Chris Beaven

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedDesign decision needed

Looks good, but a bit too much for my poor head to take in at the moment. I'll let a core developer look closer.

comment:2 Changed 10 years ago by Malcolm Tredinnick

Owner: changed from Adrian Holovaty to Malcolm Tredinnick
Triage Stage: Design decision neededAccepted

The motivation looks correct. I'm not sure if the patch is the ideal way to go about changing this behaviour (I'm not that it isn't the right way either; I need to let it bounce around in my head a bit first), but it's worth looking at. Thanks, Brian.

comment:3 Changed 10 years ago by (removed)

@mtreddinick; any further thoughts on it?

I'll admit the backwards compatibility approach (resolve_variable generating the instance, and invoking resolve) is kind of ugly, but don't see any other alternatives.

comment:4 Changed 9 years ago by (removed)

*bump* progress on this? switched to accepted 3 months ago, nothing further afaict however.

comment:5 Changed 9 years ago by Malcolm Tredinnick

Please don't bump tickets. It's still open and we'll get to it eventually. It's not a critical fix and has been prioritised accordingly.

Changed 9 years ago by (removed)

Attachment: resolve-variable.patch added

updated version since previous no longer applies; depends on var lookup needs, but seeing around 41% drop in simple var resolution (single to double lookup).

Changed 9 years ago by (removed)

Attachment: resolve-variable.2.patch added

Round two, since trac doesn't seem to like bzr bundles and since some extra crap slipped into the patch

comment:6 Changed 9 years ago by (removed)

Keywords: performance added

comment:7 Changed 9 years ago by Malcolm Tredinnick

I've given this a second-pass review now. I'm feeling happier about it; there don't appear to be any unintended caching side-effects. For example, it's still possible to return a random value from a method on an object and have it give different answers each time.

Will give it another review in a day or two and commit then if nothing pops up.

Changed 9 years ago by (removed)

correct bitrot (again)

comment:8 Changed 9 years ago by (removed)

bzr branch exists at http://pkgcore.org/~ferringb/django/resolve_variable with these changes; please pull from there next time around if it bitrots again.

Changed 9 years ago by Jacob

Attachment: resolve-variable-v3.diff added

An updated and somewhat improved version of the patch in which Variable becomes an actual class.

comment:9 Changed 9 years ago by Jacob

Owner: changed from nobody to Jacob
Status: newassigned

comment:10 Changed 9 years ago by Jacob

Resolution: fixed
Status: assignedclosed

(In [6399]) Fixed #3453: introduced a new template variable resolution system by Brian Harring (thanks!). The upshot is that variable resolution is about 25% faster, and you should see a measurable performance increase any time you've got long or deeply nested loops.

Variable resolution has changed behind the scenes -- see the note in templates_python.txt -- but template.resolve_variable() still exists. This should be fully backwards-compatible.

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