Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:49582
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain webstart.fr from 209.85.214.42 cause and error)
MIME-Version: 1.0
Sender: pierrick@webstart.fr
In-Reply-To: <4C873C0F.1010200@zend.com>
References: <AANLkTimuWDV=9AA63_9M6cQSp9riCPE214sUNdoV3tQn@mail.gmail.com>
	<4C873C0F.1010200@zend.com>
Date: Wed, 8 Sep 2010 09:05:09 -0400
Message-ID: <AANLkTimdCnW3ABq5QSnBa78H14e2LPBa=Kt2txU0sTD4@mail.gmail.com>
To: Dmitry Stogov <dmitry@zend.com>
Cc: guilhermeblanco@hotmail.com, Zeev Suraski <zeev@zend.com>, 
	PHP Internals <internals@lists.php.net>
Content-Type: text/plain; charset=ISO-8859-1
Subject: Re: PHP Annotations RFC + Patch
From: pierrick@php.net (Pierrick Charron)

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 <dmitry@zend.com>:
> 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.
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.

> 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

> 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. 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.

> 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.

> 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)
>