Vitja Makarov, 06.12.2010 09:56:
> 2010/12/6 Stefan Behnel:
>> Vitja Makarov, 05.12.2010 20:12:
>>> def my_generator(self, args, kwargs):
>>> closure = my_generator_closure_new()
>>> closure.is_running = 0
>>> closure.resume_label = 0
>>> closure.args = args
>>> closure.kwargs = kwargs
>>> closure.outer_scope = self
>>> closure.generator_body = my_generator_body
>>> return closure
>>>
>>> generator body:
>>>
>>> def my_generator_body(closure, value, is_exception):
>>
>> Shouldn't that be a cdef function?
>
> Guess no. It has very special signature and should be used only in
> generator closure.
>
> It can't be cdef as cdef doesn't parse (args, kwargs) and generator
> does this on first run.
Generators, yes. Generator expressions, no. I've written in the CEP that it
would be nice to start with splitting the way Python functions work. I
don't ask you to do this (certainly makes a nice optimisation when the
functionality itself is done), but it would easily enable a cdef function
here, which would hugely reduce the burden when advancing a generator.
>>> About temps:
>>> now temps are allocated in closure using declare_var() and it works fine.
>>
>> I think that could seriously slow down things inside of the generator,
>> though. It means that it needs to jump through the closure indirection for
>> every little value that it keeps around temporarily, including lots of
>> intermediate C results. Also, I would expect that a lot more temps are used
>> inside of the generator body than what actually needs to be kept alive
>> during yields, so a single yield inside of a lengthy generator body could
>> have a tremendous impact on the overall runtime and closure size.
>>
>> If you only store away the live temp values before a yield, you don't even
>> need to do any reference counting for them. It's just a quick bunch of C
>> assignments on yield and resume. That's about the smallest impact you can
>> get.
>>
>> BTW, if you change the object struct of the generator closure anyway, maybe
>> a dynamically sized PyVarObject would be a way to incorporate the temps?
>
> I don't understand why do you think about performance problems?
>
> It seems to me that if temps are stored in tuple (or another kind of
> object) you have to manully save/restore them, and this code will
> really slow down generator.
But it only has to be done when suspending/resuming, not every time a temp
is used. Given that there is quite some overhead involved in executing a
yield/resume cycle, it doesn't matter if we add a couple of fast C
assignments to that.
> On the other hand when all temps are declared in closure you just have
> to say "closure->pyx_temp_v1"
>
> Do you think that closure-> will be much slower then stack operations?
Yes, *much* slower. Temps are not meant to live on the stack. Most of the
time, they will just live in a CPU register, especially on x86_64 and other
architectures with plenty of registers. When you put them into the closure,
the C compiler must assume that it's an external operation when accessing
them and can't know that the next time it reads the variable it still has
the same value that it just wrote. That's kills tons of possible C
optimisations.
>>> struct closure {
>>> /* abstract_generator */
>>> PY_OBJECT_HEAD
>>> /* generator internal vars */
>>> PyObject *(*body)(struct closure *, PyObject *, int);
>>> int resume_label;
>>> ,,,,
>>> /* Closure stuff */
>>> } ;
>>>
>>> and generator utility function will work like this:
>>>
>>> PyObject *abstract_generator_next(PyObject *self, PyObject *args)
>>> {
>>> struct abstract_generator *generator = (struct abstract_generator *)
>>> self;
>>> PyObject *retval;
>>> // check running
>>> retval = generator->body(self, None, 0);
>>> // unlock running
>>> return retval;
>>> }
>>>
>>> But this way should be much better, don't know if that is possible btw.
>>>
>>> struct closure {
>>> struct abstract_generator generator;
>>> /* closure stuff here */
>>> ...
>>> } ;
>>
>> That's called inheritance in Cython. Just declare a subtype of the specific
>> closure class.
>
> That's not inheritance exactly I don't want to call all cython&
> python stuff for base class (why not btw?)
Right, why not? Closures are instantiated by calling tp_new() directly, so
even the overhead of creating them as a subtype is pretty low. Plus, it
only has to be done once when instantiating the generator, not every time
it advances. So I consider it totally negligible over the overall lifetime.
> Just want to have some fields (now I think that is_running and body is
> enough) on top of closure stuff, and I don't want to declare things
> twice.
Then inheritance is your friend. It will create a vtable, though, if you
use methods.
>>> I add MarkGenerator visitor, that sets is_generator flag on DefNode
>>> and checks for returns/yields.
>>
>> Why do you need a new class here? Can't the MarkClosureVisitor handle this?
>> It's not like transform runs over the whole AST come for free...
>
> Now I check for yield/returns inside MarkGenerator, and set
> is_generator flag or raise error if both yield and return was found.
> Guess that's not right place for this.
It's fine, but I think it's better to merge that with the
MarkClosureVisitor. It's a very related purpose. Having lots of transforms
for tiny things is just too large a burden for the compiler.
Stefan
_______________________________________________
Cython-dev mailing list
Cython-dev@codespeak.net
http://codespeak.net/mailman/listinfo/cython-dev
