Hi Guilherme,
Guilherme Blanco wrote:
Hi Dmitry,
Comments goes inline.
On Wed, Sep 8, 2010 at 10:56 AM, Dmitry Stogov <dmi...@zend.com> 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 <dmi...@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.
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.<pid>
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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
