Edgewall Software
Modify

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#295 closed enhancement (fixed)

[PATCH] Small optimization inTemplate._flatten

Reported by: Christoph Zwerschke <cito@…> Owned by: cmlenz
Priority: minor Milestone: 0.6
Component: Template processing Version: devel
Keywords: Cc:

Description

I noticed two small issues in the loop in Template._flatten:

  1. Is it clear that substream is either a list or a string and not any other kind of sequence? If not, shouldn't be other sequences treated like lists?
  2. In the case that substream is a (unicode) string, it is reassembled by joining all its characters (in this line).

The attached patch fixes these problems.

Attachments (1)

eval.patch (9.1 KB) - added by Christoph Zwerschke <cito@…> 8 years ago.
Patch for genshi.template.base

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by Christoph Zwerschke <cito@…>

Patch for genshi.template.base

comment:1 in reply to: ↑ description Changed 8 years ago by anonymous

  • Owner changed from cmlenz to anonymous
  • Status changed from new to assigned
  • Version changed from 0.5.1 to devel

Replying to Christoph Zwerschke <cito@…>:

I noticed two small issues in the loop in Template._flatten:

  1. Is it clear that substream is either a list or a string and not any other kind of sequence? If not, shouldn't be other sequences treated like lists?

Yeah, that's clear, as that's what MarkupTemplate._interpolate_attrs will create. The switch on type(x) is list instead of not isinstance(x, basestring) is a tiny optimization that can make a difference when you have many static attributes in a template.

  1. In the case that substream is a (unicode) string, it is reassembled by joining all its characters (in this line).

Ugh, good catch!

comment:2 Changed 8 years ago by cmlenz

anonymous was me

comment:3 Changed 8 years ago by cmlenz

  • Owner changed from anonymous to cmlenz
  • Status changed from assigned to new

comment:4 Changed 8 years ago by cmlenz

  • Status changed from new to assigned

comment:5 Changed 8 years ago by cmlenz

  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed in [1033].

comment:6 Changed 8 years ago by Christoph Zwerschke <cito@…>

Btw, since we're already at it, has Genshi now given up backward compatibility with Py 2.3? If yes, then using generator expressions instead of building lists may gain some efficiency. In this case, you could write

    value = u''.join(event[1]
        for event in self._flatten(value, ctxt, **vars)
        if event[0] is TEXT and event[1] is not None)
    if not value:
        continue

instead of

    values = []
    for event in self._flatten(value, ctxt, **vars):
        if event[0] is TEXT:
            values.append(event[1])
    value = [x for x in values if x is not None]
    if not value:
        continue
    value = u''.join(value)

I think the generator expression is even easier to read.

comment:7 Changed 8 years ago by cmlenz

There's a subtle difference here: in the generator expression version, you'll completely drop attributes that evaluate to an empty string. The existing implementation only drops attributes that evaluate to None.

Also, in my experience generator expressions tend to be slower than the equivalent list comprehensions for small data sets.

But that code should be converted to a single list comprehension (instead of a loop plus a list comprehension). I've checked that in in [1034].

comment:8 Changed 8 years ago by Christoph Zwerschke <cito@…>

You're right about the None/empty issue, this can be better checked with the list comprehension.

You're also right about the performance advantage of list comprehensions. I just did some quick testing with timeit. It turned out that even for larger data sets, list comprehensions were significantly faster than their corresponding generator expressions. That was not what I expected. The reason is probably that the implementation of list comprehensions is optimized and doesn't call eval_frame() as it happens for generator expression.

comment:9 Changed 8 years ago by Christoph Zwerschke <cito@…>

Btw, I repeated these tests with different Python versions from 2.4 to 3.1, with the same result. And just for fun I tested this with Iron Python as well. Turned out that generator expressions are dramatically slower in Iron Python (really ridiculously slow). So yet another reason to favor list comprehensions. But I think that's an ugly flaw in Python (and even more so in Iron Python).

Add Comment

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain cmlenz.
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.