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.
