Edgewall Software

Ticket #295 (closed enhancement: fixed)

Opened 6 years ago

Last modified 6 years ago

[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

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

Change History

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

Patch for genshi.template.base

in reply to: ↑ description   Changed 6 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.

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

Ugh, good catch!

  Changed 6 years ago by cmlenz

anonymous was me

  Changed 6 years ago by cmlenz

  • status changed from assigned to new
  • owner changed from anonymous to cmlenz

  Changed 6 years ago by cmlenz

  • status changed from new to assigned

  Changed 6 years ago by cmlenz

  • status changed from assigned to closed
  • resolution set to fixed

Fixed in [1033].

  Changed 6 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.

  Changed 6 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].

  Changed 6 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.

  Changed 6 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/Change #295 ([PATCH] Small optimization inTemplate._flatten)

Author


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


Change Properties
<Author field>
Action
as closed
The resolution will be deleted. Next status will be 'reopened'
 
Note: See TracTickets for help on using tickets.