Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:49590 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 90790 invoked from network); 9 Sep 2010 06:22:43 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 9 Sep 2010 06:22:43 -0000 Authentication-Results: pb1.pair.com smtp.mail=dmitry@zend.com; spf=pass; sender-id=pass Authentication-Results: pb1.pair.com header.from=dmitry@zend.com; sender-id=pass Received-SPF: pass (pb1.pair.com: domain zend.com designates 212.25.124.185 as permitted sender) X-PHP-List-Original-Sender: dmitry@zend.com X-Host-Fingerprint: 212.25.124.185 il-mr1.zend.com Received: from [212.25.124.185] ([212.25.124.185:41923] helo=il-mr1.zend.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id 7A/2D-33683-03D788C4 for ; Thu, 09 Sep 2010 02:22:41 -0400 Received: from il-gw1.zend.com (unknown [10.1.1.22]) by il-mr1.zend.com (Postfix) with ESMTP id 68FF550467; Thu, 9 Sep 2010 09:20:51 +0300 (IDT) Received: from ws.home (10.1.10.10) by il-ex2.zend.net (10.1.1.22) with Microsoft SMTP Server id 14.0.689.0; Thu, 9 Sep 2010 09:22:26 +0300 Message-ID: <4C887D2B.2000605@zend.com> Date: Thu, 9 Sep 2010 10:22:35 +0400 User-Agent: Thunderbird 2.0.0.23 (X11/20090825) MIME-Version: 1.0 To: Guilherme Blanco CC: Pierrick Charron , , Zeev Suraski , PHP Internals References: <4C873C0F.1010200@zend.com> <4C879613.7090709@zend.com> In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [PHP-DEV] Re: PHP Annotations RFC + Patch From: dmitry@zend.com (Dmitry Stogov) Hi Guilherme, Guilherme Blanco wrote: > 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 interesting >>>> 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 persist >>>> 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 using >>>> 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 separate >>>> CG(annotaions_map) where the keys might be the addresses of corresponding >>>> 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 case >> we accept annotations, I would represent it as a special kind of annotation. >> >> Separate map(enity -> anotations) would require memory only for defined >> annotations. Of course it would be a bit more expensive to read it and would >> 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? It would be good to separate it too, as well as different debug information e.g opline number, filename etc. > 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. he pointer takes memory too. Anyway, I think it's better to make experiment and see how this patch affects memory consumption. Just load most ZF classes and measure peak heap usage. $ USE_ZEND_ALLOC=0 valgrind --tool=massif php zf.php $ msprintf massif.out. Then we may decide if it make sense to separate annotations or not. (I can do this test and share results when have time). > Also, decomposing the annotation definition and storage adds a level > of dependency and indirection that may get things harder to comprehend > by newer contributors. Agree. >>>> 3) I think the annotaton syntax must follow PHP syntax as close as >>>> possible >>>> - allow leading \ in QualifiedName >>>> - use '=>' in arrays >>> The actual patch already implement those two points. You can right now >>> already do >>> >>> [\My\Own\Annotation(array('foo' => '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 "=" by "=>" 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( >>> name="users_phonenumbers", >>> joinColumns=array( >>> [JoinColumn(name="user_id", referencedColumnName="id")] >>> ), >>> inverseJoinColumns=array( >>> [JoinColumn(name="phonenumber_id", referencedColumnName="id", >>> unique=true)] >>> ) >>> )] > > 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 = true)])] > public $email; > } > OK. at least I don't see a reason for nested brackets. [Validation(Email(checkMX=>true))] looks better. Thanks. Dmitry. >>>> The first and second problems are the most important. >>>> I don't like to lose performance of existing applications which don't use >>>> 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 big >> 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 >> >> > > >