Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:66499
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.160.51 as permitted sender)
Sender: reeze <reeze.xia@gmail.com>
Date: Thu, 7 Mar 2013 02:14:06 +0800
To: Dmitry Stogov <dmitry@zend.com>
Cc: Nikita Popov <nikita.ppv@gmail.com>, PHP internals
 <internals@lists.php.net>, Zeev Suraski <zeev@zend.com>, Andi Gutmans
 <andi@zend.com>
Message-ID: <386313C37F2248FC953992FE658FAC5F@gmail.com>
In-Reply-To: <CA+9eiLsWNV7H1zAr1U-wPafz9SzB=eR_C33RQKgqy1Tx1+X8gw@mail.gmail.com>
References: <CAF+90c-E7n-WZcqNg28bX+G7zgbzhKoNCwR6hf++tfAgSzmtTw@mail.gmail.com>
 <CAF+90c-wSqeff_2oY8HeY+gDn-ZnwJeXKQ+cz-1W6xcEBjs+xw@mail.gmail.com>
 <CA+9eiLtiOcFuxRXhpGXe5b9SpyVPB1_ixJ4t1GQ3J1bvBURMKA@mail.gmail.com>
 <CAF+90c92AwjX7Niro_tT5vOv+vHeY-AmgfizL5f9i1FYhKTz2A@mail.gmail.com>
 <CA+9eiLsWNV7H1zAr1U-wPafz9SzB=eR_C33RQKgqy1Tx1+X8gw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/alternative; boundary="5137876e_643c9869_5d0c"
Subject: =?utf-8?Q?=E5=9B=9E=E5=A4=8D=EF=BC=9A_?=[PHP-DEV] Re: [VOTE]
 Allow non-scalar keys in foreach
From: reeze@php.net (Reeze Xia)

--5137876e_643c9869_5d0c
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline

Hi Nikita,
    I got some test failure with the patch, one test failure and some mem=
ory leaks.
see: https://gist.github.com/reeze/5101596


-- =20
reeze =7C reeze.cn

=E5=B7=B2=E4=BD=BF=E7=94=A8 Sparrow (http://www.sparrowmailapp.com/=3Fsig=
) =20

=E5=9C=A8 2013=E5=B9=B43=E6=9C=887=E6=97=A5=E6=98=9F=E6=9C=9F=E5=9B=9B=EF=
=BC=8C=E4=B8=8A=E5=8D=881:28=EF=BC=8CDmitry Stogov =E5=86=99=E9=81=93=EF=BC=
=9A

> On Wed, Mar 6, 2013 at 9:16 PM, Nikita Popov <nikita.ppv=40gmail.com (m=
ailto:nikita.ppv=40gmail.com)> wrote:
> =20
> > On Wed, Mar 6, 2013 at 5:41 PM, Dmitry Stogov <dmitry=40zend.com (mai=
lto:dmitry=40zend.com)> wrote:
> > =20
> > > Hi Nikita,
> > > =20
> > > few notes about the patch...
> > > =20
> > > - you may avoid estrndup() in zend=5Fhash=5Fcurrent=5Fkey=5Fzval=5F=
ex() for
> > > interned strings.
> > > =20
> > =20
> > =20
> > Good idea, I'll do that.
> > =20
> > > =20
> > > - I didn't completely get why did you change the =22key=22 operand =
type from
> > > IS=5FTMP=5FVAR to IS=5FVAR and how it affects performance
> > > As I understood now you need to allocate new zval on each loop iter=
ation
> > > even for foreach over plain arrays. :(
> > > =20
> > =20
> > =20
> > Good point, I didn't consider the performance implication the type ch=
ange
> > would have. The intent behind that was to avoid copying the get=5Fcur=
rent=5Fkey
> > zval into the temporary (and destroying it then), but I didn't consid=
er how
> > it affects normal arrays. This should be changed back to TMP=5FVAR.
> > =20
> =20
> =20
> 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=
.
> =20
> =20
> > =20
> > I wonder what would be a good way to avoid allocating a temporary zva=
l for
> > the key and freeing it again. Do you think it would be okay to pass
> > &EX=5FT((opline+1)->result.var).tmp=5Fvar into ->get=5Fcurrent=5Fkey(=
) so the value
> > can be written directly into it without doing extra allocs/frees=3F
> > =20
> =20
> I'm not sure it'll work. TMP=5FVARs 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=
=5FVAR
> and call copy=5Fctor().
> =20
> Please, let me review the patch when it's ready (I won't be available o=
n
> March 8 and weekend).
> =20
> Thanks. Dmitry. =20


--5137876e_643c9869_5d0c--