Hi,
Please take a look into the patch
https://github.com/php/php-src/pull/970/files
This real changes are in zend_types.h, the rest is renaming that in most
cases makes code cleaner.
zend_array didn't change its binary representation, but now it's not
possible to get a pointer to embedded HashTable. The same zend_array shoukd
be used instead.
Each HashTable got an extra 64-bit zend_refcounted header. This leads to
some increase in memory consumption.
The performance is slightly increased (may be measured with callgrind).
The patch beaks one test (tests/lang/foreachLoopObjects.006.phpt), but
actually it just disclose a problem that we have anyway.
The patch should be a base for the future optimizations. e.g. removing
HashTable->arData and/or HashTable->arHash and allocating them together
with zend_array; introducing EG(empty_array) etc.
Opinions are welcome...
Thanks. Dmitry.
De : Dmitry Stogov [mailto:dmitry@zend.com]
Please take a look into the patch
https://github.com/php/php-src/pull/970/files
This real changes are in zend_types.h, the rest is renaming that in most
cases makes code cleaner.zend_array didn't change its binary representation, but now it's not
possible to get a pointer to embedded HashTable. The same zend_array
shoukd
be used instead.Each HashTable got an extra 64-bit zend_refcounted header. This leads to
some increase in memory consumption.The performance is slightly increased (may be measured with callgrind).
The patch beaks one test (tests/lang/foreachLoopObjects.006.phpt), but
actually it just disclose a problem that we have anyway.The patch should be a base for the future optimizations. e.g. removing
HashTable->arData and/or HashTable->arHash and allocating them together
with zend_array; introducing EG(empty_array) etc.
"Showing 46 changed files with 193 additions and 204 deletions". I am sorry but I can't analyze the impacts of your change, just from the patch. Did you write an RFC that would explain the impact, especially for extension writers ?
More generally, what is the rule ? Is there a list of people who can introduce changes in the code, and even BC breaks in the C API, without writing RFCs, or is it just a question of rights/karma on the source repository ? I say that because it is especially hard to have a proposed change accepted on the mailing list, as several 'watchdogs' are there to dismiss ideas coming from people they don't know.
Hey François,
"Showing 46 changed files with 193 additions and 204 deletions". I am sorry but I can't analyze the impacts of your change, just from the patch. Did you write an RFC that would explain the impact, especially for extension writers ?
Is that because of the patch’s size? 200+/- isn’t that big for a patch.
More generally, what is the rule ? Is there a list of people who can introduce changes in the code, and even BC breaks in the C API, without writing RFCs, or is it just a question of rights/karma on the source repository ? I say that because it is especially hard to have a proposed change accepted on the mailing list, as several 'watchdogs' are there to dismiss ideas coming from people they don't know.
I don’t think there’s a hard and fast rule. We usually don’t worry as much about internal API and ABI breaks as we do about userland BC breaks, and in this case this is for a major version, so significant internal breakage is probably acceptable.
Thanks.
Andrea Faulds
http://ajf.me/
Hey François,
On 31 Dec 2014, at 17:50, François Laupretre francois@tekwire.net
wrote:"Showing 46 changed files with 193 additions and 204 deletions". I am
sorry but I can't analyze the impacts of your change, just from the patch.
Did you write an RFC that would explain the impact, especially for
extension writers ?Is that because of the patch’s size? 200+/- isn’t that big for a patch.
Well, size of a patch has no relation with the impact it could have.
More generally, what is the rule ? Is there a list of people who can
introduce changes in the code, and even BC breaks in the C API, without
writing RFCs, or is it just a question of rights/karma on the source
repository ? I say that because it is especially hard to have a proposed
change accepted on the mailing list, as several 'watchdogs' are there to
dismiss ideas coming from people they don't know.I don’t think there’s a hard and fast rule. We usually don’t worry as
much about internal API and ABI breaks as we do about userland BC breaks,
and in this case this is for a major version, so significant internal
breakage is probably acceptable.
Php7 breaks everything anyway, so no big deal here.
However at least UPGRADING.INTERNALS should be done. A couple of things are
already missing and it is going to be hard to keep it synced.
Cheers,
Pierre
Hi François,
this is not a proposal yet, this is just an initial request to the most
experinced PHP developers to think about possible consequnces.
Thanks. Dmitry.
De : Dmitry Stogov [mailto:dmitry@zend.com]
Please take a look into the patch
https://github.com/php/php-src/pull/970/files
This real changes are in zend_types.h, the rest is renaming that in most
cases makes code cleaner.zend_array didn't change its binary representation, but now it's not
possible to get a pointer to embedded HashTable. The same zend_array
shoukd
be used instead.Each HashTable got an extra 64-bit zend_refcounted header. This leads to
some increase in memory consumption.The performance is slightly increased (may be measured with callgrind).
The patch beaks one test (tests/lang/foreachLoopObjects.006.phpt), but
actually it just disclose a problem that we have anyway.The patch should be a base for the future optimizations. e.g. removing
HashTable->arData and/or HashTable->arHash and allocating them together
with zend_array; introducing EG(empty_array) etc."Showing 46 changed files with 193 additions and 204 deletions". I am
sorry but I can't analyze the impacts of your change, just from the patch.
Did you write an RFC that would explain the impact, especially for
extension writers ?More generally, what is the rule ? Is there a list of people who can
introduce changes in the code, and even BC breaks in the C API, without
writing RFCs, or is it just a question of rights/karma on the source
repository ? I say that because it is especially hard to have a proposed
change accepted on the mailing list, as several 'watchdogs' are there to
dismiss ideas coming from people they don't know.
Hi Dmitry,
Hi,
Please take a look into the patch
https://github.com/php/php-src/pull/970/files
This real changes are in zend_types.h, the rest is renaming that in most
cases makes code cleaner.zend_array didn't change its binary representation, but now it's not
possible to get a pointer to embedded HashTable. The same zend_array
shoukd be used instead.Each HashTable got an extra 64-bit zend_refcounted header. This leads to
some increase in memory consumption.The performance is slightly increased (may be measured with callgrind).
The patch beaks one test (tests/lang/foreachLoopObjects.006.phpt), but
actually it just disclose a problem that we have anyway.The patch should be a base for the future optimizations. e.g. removing
HashTable->arData and/or HashTable->arHash and allocating them together
with zend_array; introducing EG(empty_array) etc.Opinions are welcome...
Thanks. Dmitry.
I've tested the patch on Windows x64 TS build. On my laptop the profiler
tells 8.5m more instructions retired. I guess that's because of less
dereferencing happening (it operates faster and pipelines have more idle
time). Also there's less branch misprediction and a negligible memory
usage increase. All in all - the patch is probably more like an
intermediate step to the further optimization you mention. But even in the
current patch state - there's no impact.
Regards
Anatol
Thanks for analizing.
did you say "8.5 more" instructions?
in my test I saw "less".
It's really an intermedie step, and we probably will check if other
optimizations are reaaly benefitable before committing this. But yes, this
patch doesn't make any degradation, except for slight increase of memory
consumption.
Thanks. Dmitry.
Hi Dmitry,
Hi,
Please take a look into the patch
https://github.com/php/php-src/pull/970/files
This real changes are in zend_types.h, the rest is renaming that in most
cases makes code cleaner.zend_array didn't change its binary representation, but now it's not
possible to get a pointer to embedded HashTable. The same zend_array
shoukd be used instead.Each HashTable got an extra 64-bit zend_refcounted header. This leads to
some increase in memory consumption.The performance is slightly increased (may be measured with callgrind).
The patch beaks one test (tests/lang/foreachLoopObjects.006.phpt), but
actually it just disclose a problem that we have anyway.The patch should be a base for the future optimizations. e.g. removing
HashTable->arData and/or HashTable->arHash and allocating them together
with zend_array; introducing EG(empty_array) etc.Opinions are welcome...
Thanks. Dmitry.
I've tested the patch on Windows x64 TS build. On my laptop the profiler
tells 8.5m more instructions retired. I guess that's because of less
dereferencing happening (it operates faster and pipelines have more idle
time). Also there's less branch misprediction and a negligible memory
usage increase. All in all - the patch is probably more like an
intermediate step to the further optimization you mention. But even in the
current patch state - there's no impact.Regards
Anatol
Hi,
Thanks for analizing.
did you say "8.5 more" instructions? in my test I saw "less".It's really an intermedie step, and we probably will check if other
optimizations are reaaly benefitable before committing this. But yes, this
patch doesn't make any degradation, except for slight increase of memory
consumption.
yeah, i'm afraid that's a typo, less instruction retired. Here's the full
report I have, rerun the profiling a couple of times again to be sure. The
numbers on the right are from the patch and on the left from the
mainstream.
Elapsed Time: 2.428s - 2.088s = 0.339s
Clockticks: 5,891,879,618 - 5,800,714,085 = 91,165,533
Instructions Retired: 10,148,429,042 - 10,121,040,536 = 27,388,506
CPI Rate: 0.581 - 0.573 = 0.007
MUX Reliability: 0.985 - 0.977 = 0.008
Paused Time: 0.263s - 0s = 0.263s
Filled Pipeline Slots:
Retiring: 0.483 - 0.488 = -0.006
Assists: 0.000 - 0.000 = 0.000
Bad Speculation: 1.000 - 0.890 = 0.110
Branch Mispredict: 1.000 - 1.000 = 0.000
Machine Clears: 0.000 - 0.000 = 0.000
Unfilled Pipeline Slots (Stalls):
Back-end Bound: 0.049 - 0.105 = -0.056
DIV Active: 0.000 - 0.000 = 0.000
Flags Merge Stalls: 0.006 - 0.000 = 0.006
Slow LEA Stalls: 0.006 - 0.000 = 0.006
Memory Latency:
LLC Miss: 0.000 - 0.000 = 0.000
LLC Hit: 0.000 - 0.000 = 0.000
DTLB Overhead: 0.000 - 0.001 = -0.001
Contested Accesses: 0.000 - 0.000 = 0.000
Data Sharing: 0.000 - 0.000 = 0.000
Memory Replacements:
L1D Replacement Percentage: 0.000 - 0.000 = 0.000
L2 Replacement Percentage: 1.000 - 1.000 = 0.000
LLC Replacement Percentage: 0.000 - 0.000 = 0.000
Memory Reissues:
Loads Blocked by Store Forwarding: 0.000 - 0.000 = 0.000
Split Loads: 0.000 - 0.000 = 0.000
Split Stores: 0.000 - 0.000 = 0.000
4K Aliasing: 0.011 - 0.013 = -0.003
Front-end Bound: 0.137 - 0.100 = 0.037
ICache Misses: 0.000 - 0.000 = 0.000
ITLB Overhead: 0.000 - 0.000 = 0.000
DSB Switches: 0.000 - 0.000 = 0.000
As one sees, the instruction count vary, but that's most likely because of
the measurement uncertainty.
Regards
Anatol
Hi,
Please take a look into the patch
https://github.com/php/php-src/pull/970/files
This real changes are in zend_types.h, the rest is renaming that in most
cases makes code cleaner.zend_array didn't change its binary representation, but now it's not
possible to get a pointer to embedded HashTable. The same zend_array shoukd
be used instead.Each HashTable got an extra 64-bit zend_refcounted header. This leads to
some increase in memory consumption.
In your patch zend_hash_init (and zend_hash_destroy as well) ignores the
refcounted header. So currently you wouldn't be able to use just any
HashTable* in a refcounted way (e.g. do an object->array cast by adding a
ref to the properties HT and sticking it in a zval). You can only do the
reverse, i.e. use a no longer used refcounted array somewhere else (like
the way properties are assigned in pdo/... now).
What's the plan about this? Is this intentional or will the hash API start
managing the refcount as well (like strings already do)?
The performance is slightly increased (may be measured with callgrind).
The patch beaks one test (tests/lang/foreachLoopObjects.006.phpt), but
actually it just disclose a problem that we have anyway.The patch should be a base for the future optimizations. e.g. removing
HashTable->arData and/or HashTable->arHash and allocating them together
with zend_array; introducing EG(empty_array) etc.
Could you elaborate on the first part? How is it possible to alloc
arData/arHash together with zend_array? (Or rather, if you do that, how can
you change the size later?)
Nikita
Hi Nikita,
Hi,
Please take a look into the patch
https://github.com/php/php-src/pull/970/files
This real changes are in zend_types.h, the rest is renaming that in most
cases makes code cleaner.zend_array didn't change its binary representation, but now it's not
possible to get a pointer to embedded HashTable. The same zend_array shoukd
be used instead.Each HashTable got an extra 64-bit zend_refcounted header. This leads to
some increase in memory consumption.In your patch zend_hash_init (and zend_hash_destroy as well) ignores the
refcounted header. So currently you wouldn't be able to use just any
HashTable* in a refcounted way (e.g. do an object->array cast by adding a
ref to the properties HT and sticking it in a zval). You can only do the
reverse, i.e. use a no longer used refcounted array somewhere else (like
the way properties are assigned in pdo/... now).What's the plan about this? Is this intentional or will the hash API
start managing the refcount as well (like strings already do)?
you are right. It must be fixed in some way. we may start playing adding
assert(refcount == 1) in all zend_hash functions that modifyes HashTables.
Other ideas are wecome...
The performance is slightly increased (may be measured with callgrind).
The patch beaks one test (tests/lang/foreachLoopObjects.006.phpt), but
actually it just disclose a problem that we have anyway.The patch should be a base for the future optimizations. e.g. removing
HashTable->arData and/or HashTable->arHash and allocating them together
with zend_array; introducing EG(empty_array) etc.Could you elaborate on the first part? How is it possible to alloc
arData/arHash together with zend_array? (Or rather, if you do that, how can
you change the size later?)
We may reallocate zend_array itself. Of course, it would require using
zend_array ** in all functions that may modify it, and also using
zend_array* instead of embeded HashTables.
Thanks. Dmitry.
Nikita