On Wed, Mar 6, 2013 at 9:16 PM, Nikita Popov <nikita....@gmail.com> wrote:
> On Wed, Mar 6, 2013 at 5:41 PM, Dmitry Stogov <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.
