Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:49584 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 97199 invoked from network); 8 Sep 2010 16:44:49 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 8 Sep 2010 16:44:49 -0000 Authentication-Results: pb1.pair.com smtp.mail=guilhermeblanco@gmail.com; spf=pass; sender-id=pass Authentication-Results: pb1.pair.com header.from=guilhermeblanco@gmail.com; sender-id=pass; domainkeys=bad Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.215.170 as permitted sender) DomainKey-Status: bad X-DomainKeys: Ecelerity dk_validate implementing draft-delany-domainkeys-base-01 X-PHP-List-Original-Sender: guilhermeblanco@gmail.com X-Host-Fingerprint: 209.85.215.170 mail-ey0-f170.google.com Received: from [209.85.215.170] ([209.85.215.170:61930] helo=mail-ey0-f170.google.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id B1/06-61499-F7DB78C4 for ; Wed, 08 Sep 2010 12:44:48 -0400 Received: by eyg24 with SMTP id 24so223889eyg.29 for ; Wed, 08 Sep 2010 09:44:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:received:in-reply-to :references:date:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=Kso5LI2zVpDdYTDqJF0wD+bRoldrehvGew6+2bFJKJw=; b=EkryIby/BRMorn3U3gnvtufgMx3gA6RseS2pdgJRpB6Q9Y2LOFZzEqmuMI7ApAAJaC VsUG34uAV0OqkBO0e6zvOOAkPcfDSx5kV8BUIPy+b1yxtE7xKfE3X8P2//aO1vKCFlmj Z+fouxxCw6/Jh2UU5dfvYJ6E3DpTMYJDj0sdE= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=m/8xrdiN+u4GzCxCBQrqbJMpuifX7cWmv3UX+Wc8hvohx7LU7TK/+LgYBFMq66BKOn UoQpIaWtZY33Q5OSX/pSThKJNGFRKSXtkYSCX5WpZzKpAtyhDzCUVdHvzzufr45P51h+ iGI/x7sGhqnQJKdnC2tOHrt7N/A467mUIrlcU= MIME-Version: 1.0 Received: by 10.216.174.69 with SMTP id w47mr92696wel.25.1283964284205; Wed, 08 Sep 2010 09:44:44 -0700 (PDT) Received: by 10.216.55.130 with HTTP; Wed, 8 Sep 2010 09:44:44 -0700 (PDT) In-Reply-To: <4C879613.7090709@zend.com> References: <4C873C0F.1010200@zend.com> <4C879613.7090709@zend.com> Date: Wed, 8 Sep 2010 13:44:44 -0300 Message-ID: To: Dmitry Stogov Cc: Pierrick Charron , guilhermeblanco@hotmail.com, Zeev Suraski , PHP Internals Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [PHP-DEV] Re: PHP Annotations RFC + Patch From: guilhermeblanco@gmail.com (Guilherme Blanco) Hi Dmitry, Comments goes inline. On Wed, Sep 8, 2010 at 10:56 AM, Dmitry Stogov wrote: > > > Pierrick Charron wrote: >> >> Hi Dmitry, >> >> First thanks for having a look at the patch and taking some time to >> give your feedback ! It's much appreciated >> >> 2010/9/8 Dmitry Stogov : >>> >>> Hi Pierrick, >>> >>> I've taken just a quick look into concept and patch. It looks interesti= ng >>> and might be useful in some areas, but I see several significant >>> problems: >>> >>> 1) Have you thought about compatibility with opcode caches? In case I >>> understood properly, you store annotation as a HashTable of object. But >>> life-time of objects is restricted by request time, so they won't persi= st >>> in >>> opcode cache and will have to be recreated on each request. It's a huge >>> overhead. I don't have a good solution for this problem except for usin= g >>> arrays instead of objects. >> >> You're right annotations are stored in an HashTable. But this >> HashTable doesn't store zval objects directly but zend_annotations. > > Then it's not a problem. my mistake. > >> This is the reflection getAnnotation and getAnnotations method which >> are in charge of reading the zend_annotation structures and >> instantiating the proper annotation object when needed. The zval named >> instance in the zend_annotation structure is there only to point to >> the annotation instance at runtime so that the state of the annotation >> is kept between two calls to the getAnnotation and getAnnotations on a >> same code element (I added this directly into the zend_annotation >> structure to make it faster but I can imagine a HashTable like you >> mentioned earlier where keys are addresses). I'm not yet quite >> familiar with the APC source code but since (and correct me if i'm >> wrong) APC save zend_class_entry, etc.. in memory we could modify it >> to also save the new HashTable *annotations of those structures (like >> you already do for doc_comment) excluding the zval *instance. >> >>> 2) I suppose that usage of annotation would be quite rare case. I don't >>> think it make sense to extend each op_array, property and class with >>> additional "annotations" field. I think it's possible to have a separat= e >>> CG(annotaions_map) where the keys might be the addresses of correspondi= ng >>> classesm, methods and properties. >> >> Annotations are metadata related to a code element. IMHO it make sense >> to add them in the appropriate code element structures (even >> doc_comment is in those structures) so that APC and other cache will >> know that this is something part of the code element and it needs to >> be cached and so on. > > It's easier to keep it in appropriate places, but it's a waste of memory, > because only a few classes will use it. According to doc comments, in cas= e > we accept annotations, I would represent it as a special kind of annotati= on. > > Separate map(enity -> anotations) would require memory only for defined > annotations. Of course it would be a bit more expensive to read it and wo= uld > require special handling in opcode hashes. Argument is not reasonable IMHO. You may know that most code around web lack of documentation, specially docblock, so why not separate the docblocks on a separate structure too? Sorry, but your argument is not reasonable. Each class/property/method should contain all relevant information it can get, whatever it needs (docblock, annotations, visibility information, etc). Of course you have a bit of memory overhead, but it's a pointer, isn't it? So at the end, we will only have overhead when we have annotations defined. Also, decomposing the annotation definition and storage adds a level of dependency and indirection that may get things harder to comprehend by newer contributors. > >>> 3) I think the annotaton syntax must follow PHP syntax as close as >>> possible >>> - allow leading \ in QualifiedName >>> - use '=3D>' in arrays >> >> The actual patch already implement those two points. You can right now >> already do >> >> [\My\Own\Annotation(array('foo' =3D> 'bar', 'baz'))] >> class FooBar {} >> >> Sorry about that, I just noticed that the EBNF in the RFC was not >> updated correctly to reflect those two points. I just made the >> modification to replace "=3D" by "=3D>" in arrays and allow the optional >> leading \ in the QualidifedName > > great. > >>> 4) I didn't understand why do we need nested annotations. >> >> Nested annotations could be use for many cases. Doctrine for example >> use them a lot to do things like : >> [JoinTable( >> =C2=A0 =C2=A0name=3D"users_phonenumbers", >> =C2=A0 =C2=A0joinColumns=3Darray( >> =C2=A0 =C2=A0 =C2=A0 =C2=A0[JoinColumn(name=3D"user_id", referencedColum= nName=3D"id")] >> =C2=A0 =C2=A0), >> =C2=A0 =C2=A0inverseJoinColumns=3Darray( >> =C2=A0 =C2=A0 =C2=A0 =C2=A0[JoinColumn(name=3D"phonenumber_id", referenc= edColumnName=3D"id", >> unique=3Dtrue)] >> =C2=A0 =C2=A0) >> )] I see many use cases of nested annotations. Pierrick gave one, that is exactly related to the project I contribute for. Another good example is the implementation of JSR-303 (Bean Validation) on Symfony 2, that adds automatic validation to POPOs. Example: class User { ... [Validation([Email(checkMX =3D true)])] public $email; } >> >>> The first and second problems are the most important. >>> I don't like to lose performance of existing applications which don't u= se >>> annotations at all. I think the concept must be redesigned a bit. >> >> Right now the only performance decrease is at compile time since the >> annotations HashTable are filled by the parser. > > I assume parser works with near the same speed without annotations. right= ? Yes. It is as complex as the addiction of any new feature that includes new grammar rules, like type hinting checks on method prototypes. > >> At runtime you don't >> have any performance decrease if you're not using annotations. If you >> use them, you'll lose some time when you do getAnnotation() to >> instantiate the object, but this instantiation is just done once for >> each annotation on a single code element so it make it a little bit >> faster if you use them intensively. > > I see, but assumptions are not always right, often memory consumption > affects performance directly, it's better to check. The impact is minimum because it only adds pointers to structs. The instantiation is done on demand, so only when user asks for Annotations they are instantiated. > >>> Also I would like to see an APC patch to check if the implementation >>> works >>> and doesn't lose performance. >> >> I will try to work on an APC patch to store the Annotations HashTable >> of every code element structure. > > In case you have plain C structures (without objects) it shouldn't be a b= ig > problem. I'm more interested to store annotation separately. In this case > APC support would be a bit more difficult. > > I could help you with implementation in case the idea accepted in general= . Thanks a lot for the offered help. Without help from core developers it would be very difficult to reach a performant proposed patch. > > Thanks. Dmitry. > >>> Thanks. Dmitry. >> >> Thanks again for your time :) >> >> Pierrick >> >>> Pierrick Charron wrote: >>>> >>>> Hi Dmitry, >>>> >>>> As discussed on IRC, here are the RFC and the patch to add Annotation >>>> support in PHP : >>>> >>>> - RFC : http://wiki.php.net/rfc/annotations >>>> - Patch : http://www.adoy.net/php/Annotations.diff >>>> >>>> All your feedback and suggestions will be more than welcome. >>>> >>>> Thanks again. >>>> >>>> Pierrick (or adoy on irc) > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > > --=20 Guilherme Blanco Mobile: +55 (16) 9215-8480 MSN: guilhermeblanco@hotmail.com S=C3=A3o Paulo - SP/Brazil