First off, great to hear you're working on this. I've wanted to do it for a while (well, essentially ever since I got closures done), but don't know when I'll find the time.
On Mon, Dec 6, 2010 at 12:56 AM, Vitja Makarov <[email protected]> wrote: >>> 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. Eventually the argument parsing code should be abstracted out so that we don't have this restriction. >>> # switch case >>> switch (closure.resume_lable): >>> case 0: >>> goto parse_arguments; >>> case 1: >>> goto resume_from_yield1; >>> parse_arguments: >>> # function body here >> >> Ok, sure, as in the CEP. >> >> >>> 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. -1 to storing them all in a tuple. That wouldn't handle C values very well, for one thing. I wouldn't use arrays though--either declare each temp individually or create a custom struct that has the correct slot for each temp. Copy to locals on enter, copy out on yield/exception/return. > 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, the performance difference can be dramatic. This is one of the reasons good numpy support is tricky to get right--the compiler can optimize stuff on the stack like crazy, but the same can't be said of indirect references. I'm fine with sticking all temps in the closure for now if that gets us up and running faster, and optimizing things later. >>> I'm not sure about manage_ref flag I guess that temps with >>> manage_ref=False shouldn't be allocated on closure (am I right?) >> >> "manage_ref=False" (for a Python object value) just means that the >> reference is either borrowed or will be stolen at some point. I'm not sure >> that stealing the reference may not happen *after* a yield, but I guess >> that would produce other problems if the reference isn't kept alive in >> another way. Nothing to worry about now, carefully crafted tests will >> eventually catch that. >> >> In any case, all live temps (Python refs, borrowed refs and C values) may >> be needed after the yield, so they must *all* end up in the closure. And of course we need to make all live reference are properly deallocated when the closure is GC'd as well. - Robert _______________________________________________ Cython-dev mailing list [email protected] http://codespeak.net/mailman/listinfo/cython-dev
