Hi Nikita, I got some test failure with the patch, one test failure and some memory leaks. see: https://gist.github.com/reeze/5101596
-- reeze | reeze.cn 已使用 Sparrow (http://www.sparrowmailapp.com/?sig) 在 2013年3月7日星期四，上午1:28，Dmitry Stogov 写道： > On Wed, Mar 6, 2013 at 9:16 PM, Nikita Popov <nikita....@gmail.com > (mailto:nikita....@gmail.com)> wrote: > > > On Wed, Mar 6, 2013 at 5:41 PM, Dmitry Stogov <dmi...@zend.com > > (mailto:dmi...@zend.com)> wrote: > > > > > Hi Nikita, > > > > > > few notes about the patch... > > > > > > - you may avoid estrndup() in zend_hash_current_key_zval_ex() for > > > interned strings. > > > > > > > > > Good idea, I'll do that. > > > > > > > > - I didn't completely get why did you change the "key" operand type from > > > IS_TMP_VAR to IS_VAR and how it affects performance > > > As I understood now you need to allocate new zval on each loop iteration > > > even for foreach over plain arrays. :( > > > > > > > > > Good point, I didn't consider the performance implication the type change > > would have. The intent behind that was to avoid copying the get_current_key > > zval into the temporary (and destroying it then), but I didn't consider how > > it affects normal arrays. This should be changed back to TMP_VAR. > > > > > It would be great. I can agree that new features may work slower, but > really don't like when they slowdown existing and mach more usual cases. > > > > > > I wonder what would be a good way to avoid allocating a temporary zval for > > the key and freeing it again. Do you think it would be okay to pass > > &EX_T((opline+1)->result.var).tmp_var into ->get_current_key() so the value > > can be written directly into it without doing extra allocs/frees? > > > > I'm not sure it'll work. TMP_VARs don't initialize refcount, they can't be > referenced or marked as a possible root of garbage. > I took only a very quick look over the patch and didn't understand all the > details, but probably it must be possible to copy iterator key into TMP_VAR > and call copy_ctor(). > > Please, let me review the patch when it's ready (I won't be available on > March 8 and weekend). > > Thanks. Dmitry.