# execute_data->Ts removing - Thread: https://externals.io/message/64120 - Original archive: https://news-web.php.net/php.internals/64120 - Started by Dmitry Stogov on 2012-11-30T14:15:36+00:00 - Messages: 9 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #64120 — Dmitry Stogov — 2012-11-30T14:15:36+00:00 URL: https://externals.io/message/64120#64120 Hi, The NEWS and UPGRADING explains the details. http://pastebin.com/VC71Y8LV The patch is big, but actually quite simple. I'm going to commit it on Monday or Tuesday (if no objections). I'm going to look into the similar optimization for CVs, but it's going to be a bit more difficult. Thanks. Dmitry. --- ## #64123 — Rasmus Lerdorf — 2012-11-30T19:48:52+00:00 URL: https://externals.io/message/64120#64123 · In reply to: #64120 > Hi, > > The NEWS and UPGRADING explains the details. > > http://pastebin.com/VC71Y8LV > > The patch is big, but actually quite simple. > I'm going to commit it on Monday or Tuesday (if no objections). > > I'm going to look into the similar optimization for CVs, but it's going to > be a bit more difficult. Looks good to me. I'll commit this change to APC when you commit it: Index: apc_zend.c =================================================================== --- apc_zend.c (revision 328577) +++ apc_zend.c (working copy) @@ -48,7 +48,11 @@ static opcode_handler_t *apc_original_opcode_handlers; static opcode_handler_t apc_opcode_handlers[APC_OPCODE_HANDLER_COUNT]; +#if `PHP_MAJOR_VERSION` >= 6 || `PHP_MAJOR_VERSION` == 5 && `PHP_MINOR_VERSION` >= 5 +#define APC_EX_T(offset) (*EX_TMP_VAR(execute_data, offset)) +#else #define APC_EX_T(offset) (*(temp_variable *)((char*)execute_data->Ts + offset)) +#endif And there are a couple of extensions that are going to need similar changes because of this. pecl/optimizer, pecl/inclued, pecl/xhprof, pecl/operator and xdebug from a quick check. -Rasmus --- ## #64134 — Dmitry Stogov — 2012-12-03T05:47:19+00:00 URL: https://externals.io/message/64120#64134 · In reply to: #64123 Hi Rasmus, I'm glad the patch is not a big problem for APC. BTW: I decided not to commit the patch as is, and implement the similar optimization for CVs first. Otherwise I may need to change the way TMP_VAR accessed twice. I'll probably send the patch for review later today. Thanks. Dmitry. > > > […] > to > > […] > > Looks good to me. I'll commit this change to APC when you commit it: > > Index: apc_zend.c > =================================================================== > --- apc_zend.c (revision 328577) > +++ apc_zend.c (working copy) > @@ -48,7 +48,11 @@ > static opcode_handler_t *apc_original_opcode_handlers; > static opcode_handler_t apc_opcode_handlers[APC_OPCODE_HANDLER_COUNT]; > > +#if `PHP_MAJOR_VERSION` >= 6 || `PHP_MAJOR_VERSION` == 5 && > `PHP_MINOR_VERSION` >= 5 > +#define APC_EX_T(offset) (*EX_TMP_VAR(execute_data, > offset)) > +#else > #define APC_EX_T(offset) (*(temp_variable > *)((char*)execute_data->Ts + offset)) > +#endif > > And there are a couple of extensions that are going to need similar > changes because of this. pecl/optimizer, pecl/inclued, pecl/xhprof, > pecl/operator and xdebug from a quick check. > > -Rasmus --- ## #64135 — Dmitry Stogov — 2012-12-03T09:35:52+00:00 URL: https://externals.io/message/64120#64135 · In reply to: #64134 The new proposed patch: http://pastebin.com/pj5fQTfN Now both execute_data->Ts and execute_data->CVs are removed and corresponding temporary and compiled variables accessed using "execute_data" as the base pointer. Temporary variables allocate directly before the "execute_data" in reverse order and compiled variables right after. So they can be accessed without any additional computations. The patch reduces the number of executed instructions and number of memory reads (about 8% less). I'm going to commit the patch on Tuesday. Thanks. Dmitry. > Hi Rasmus, > > I'm glad the patch is not a big problem for APC. > BTW: I decided not to commit the patch as is, and implement the similar > optimization for CVs first. > Otherwise I may need to change the way TMP_VAR accessed twice. > I'll probably send the patch for review later today. > > Thanks. Dmitry. > > On Fri, Nov 30, 2012 at 11:48 PM, Rasmus Lerdorf wrote: > > > […] --- ## #64136 — yoram bar haim — 2012-12-03T13:18:32+00:00 URL: https://externals.io/message/64120#64136 · In reply to: #64135 Hi Dmitry. is this improvemnt (intruction reduce and memory read reduce) preserved under "strong" compiler optimization (-O2, -O3) ? is it something that the compiler can not automatically optimize ? anyhow, the patch seems impressive... > The new proposed patch: http://pastebin.com/pj5fQTfN > > Now both execute_data->Ts and execute_data->CVs are removed and > corresponding temporary and compiled variables accessed using > "execute_data" as the base pointer. Temporary variables allocate directly > before the "execute_data" in reverse order and compiled variables right > after. So they can be accessed without any additional computations. The > patch reduces the number of executed instructions and number of memory > reads (about 8% less). > > I'm going to commit the patch on Tuesday. > > Thanks. Dmitry. > > > > […] wrote: > > […] --- ## #64137 — Dmitry Stogov — 2012-12-03T13:26:18+00:00 URL: https://externals.io/message/64120#64137 · In reply to: #64136 Hi Yoram, Yeas, I checked memory reads on a release PHP built (with -O2 optimization level). The idea is very simple. Before the patch each access to TMP was implemented as: execute_data->Ts + offset GCC with -O2 kept execute_data in register but it needed to read execute_data->Ts anyway and then add the offset Now we just add the offset to execute_data directly. Also, the patch I published today is not compatible with 64-bit systems. I already fixed it, and the fix is minimal. Thanks. Dmitry. > Hi Dmitry. > is this improvemnt (intruction reduce and memory read reduce) preserved > under > "strong" compiler optimization (-O2, -O3) ? > is it something that the compiler can not automatically optimize ? > anyhow, the patch seems impressive... > > > > […] > wrote: > > […] > apc_opcode_handlers[APC_OPCODE_HANDLER_COUNT]; > > […] > (*EX_TMP_VAR(execute_data, > > […] --- ## #64138 — jpauli — 2012-12-03T14:26:48+00:00 URL: https://externals.io/message/64120#64138 · In reply to: #64137 > Hi Yoram, > > Yeas, I checked memory reads on a release PHP built (with -O2 optimization > level). > > The idea is very simple. Before the patch each access to TMP was > implemented as: > > execute_data->Ts + offset > > GCC with -O2 kept execute_data in register but it needed to read > execute_data->Ts anyway and then add the offset > > Now we just add the offset to execute_data directly. > > Also, the patch I published today is not compatible with 64-bit systems. > I already fixed it, and the fix is minimal. > > Thanks. Dmitry. > Great work Dmitry, as usual ! Julien --- ## #64139 — Nikita Popov — 2012-12-03T15:33:55+00:00 URL: https://externals.io/message/64120#64139 · In reply to: #64135 > The new proposed patch: http://pastebin.com/pj5fQTfN > > Now both execute_data->Ts and execute_data->CVs are removed and > corresponding temporary and compiled variables accessed using > "execute_data" as the base pointer. Temporary variables allocate directly > before the "execute_data" in reverse order and compiled variables right > after. So they can be accessed without any additional computations. The > patch reduces the number of executed instructions and number of memory > reads (about 8% less). > Did you also test how much these 8% less memory reads improve performance? Would be interesting to know :) Nikita --- ## #64140 — Dmitry Stogov — 2012-12-03T16:59:56+00:00 URL: https://externals.io/message/64120#64140 · In reply to: #64139 Unfortunately, I didn't see visible performance gain on x86. :( It's near the measurement mistake. Probably it's because reading from L1 data cache is very cheap. Thanks. Dmitry. > > > > […] > > Did you also test how much these 8% less memory reads improve performance? > Would be interesting to know :) > > Nikita