Hi Derick,
Thank you for review.
Derick Rethans wrote:
On Tue, 13 Apr 2010, Dmitry Stogov wrote:
I've published all the patches, their description and performance evaluation
at http://wiki.php.net/rfc/performanceimprovements
I've a few questions and comments:
http://wiki.php.net/rfc/performanceimprovements#zend_engine_vm_tuning
You're writing that the ZEND_CATCH opcode has to be used with a constant
class name, so how would "catch ( self $e )" be represented in op-codes
now?
I dropped such possibility. Does someone use it?
For me it looks useless. If it's really necessary I can try not to drop it.
Some patches seem to be changing tests; I think we should avoid that and
instead create a new test that only runs for the new version, and add a
qualifier to the old one (the one that you're now modifying) to *not*
run with the new PHP version.
It's not a problem. I can do it.
The patch made one test to emit warning messages in different order, but
on the other hand the change made the performance better.
2-empty_hash.diff.txt seems to be ignoring some of our coding
guidelines:
if(ht->nTableMask) pefree(ht->arBuckets, ht->persistent);
should clearly be:
if (ht->nTableMask) {
pefree(ht->arBuckets, ht->persistent);
}
Could you please check the space between "if" and "(", and always
using braces for all the patches?
I'll fix it.
3-literals.diff.txt makes the opcode arrays again a bit more complex to
disect. Perhaps you could add a bit of documentation describing how all
the things work together now? Like which index in whivh fields points to
which other structure etc?
it's not a problem to add a few words about it:
- each op_array has an array of literals (constant values)
- opocde operands don't contain zval directly any more but points to
this table instead
- during compilation they are accessible by index e.g.
op_array->literals[opline->op1.constant].constant
- the pass_two() changes indexes into pointers so during execution they
are accessible by
opline->op1.zv
Do you like me to add this into WiKi or some text file?
4-interned.diff.txt
The change in array.c, is that strictly necessary, or just another
performance improvement?
It's necessary. The change disables in-place string modification.
Things like make me wonder, whether it would work to make this macro
part of efree() ? (or create an esfree() (string-free) instead?
- efree(Z_STRVAL_P(value));
+ if (!IS_INTERNED(Z_STRVAL_P(value))) {
+ efree(Z_STRVAL_P(value));
+ }
It makes sense :)
In the RFC you write "Note: The usage of special data structure (e.g.
zend_string) instead of char* throughout PHP would probably be a more
consistent solution, but it would be a huge break. We are going to look
into this solution in the future.". This would be an API break only,
right, very similar to what the Unicode patch did with zstr's?
I don't understand the *only*. Of course it would be an API break which
would need to be propagated over the whole PHP code. It has some
similarity with "zstr", but it would be more powerful structure which
contain value, length, hash_value, interned flag, may be refcount. It
also may be extended with charset/encoding to implement Unicode strings
in a Parrot way.
Thanks. Dmirty.
with kind regards,
Derick
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
