Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:49583 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 72069 invoked from network); 8 Sep 2010 13:56:45 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 8 Sep 2010 13:56:45 -0000 Authentication-Results: pb1.pair.com header.from=dmitry@zend.com; sender-id=pass Authentication-Results: pb1.pair.com smtp.mail=dmitry@zend.com; spf=pass; 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:39521] helo=il-mr1.zend.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id 1F/F1-61499-816978C4 for ; Wed, 08 Sep 2010 09:56:43 -0400 Received: from il-gw1.zend.com (unknown [10.1.1.22]) by il-mr1.zend.com (Postfix) with ESMTP id D1A155045C; Wed, 8 Sep 2010 16:54:53 +0300 (IDT) Received: from ws.home (10.1.10.4) by il-ex2.zend.net (10.1.1.22) with Microsoft SMTP Server id 14.0.689.0; Wed, 8 Sep 2010 16:56:28 +0300 Message-ID: <4C879613.7090709@zend.com> Date: Wed, 8 Sep 2010 17:56:35 +0400 User-Agent: Thunderbird 2.0.0.23 (X11/20090825) MIME-Version: 1.0 To: Pierrick Charron CC: , Zeev Suraski , PHP Internals References: <4C873C0F.1010200@zend.com> In-Reply-To: Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: PHP Annotations RFC + Patch From: dmitry@zend.com (Dmitry Stogov) 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. >> 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)] > ) > )] > >> 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? > 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. >> 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. 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)