RE: [PHP-DEV] Patch for opcode caches

Hi Marcus, I've just rechecked the "-e" option, and it works fine for me. I'm wonderwed why (and how) it didn't work for you. I hope I already answered your questions and you see the needness of this patch. So I hope you agree that I'll commit it tomorrow (with comments about compiler options) Thanks. Dmitry. > -----Original Message----- > From: Dmitry Stogov > Sent: Monday, March 10, 2008 3:48 PM > To: 'Marcus Boerger' > Cc: Andi Gutmans; Stas Malyshev; 'phpxcache'; > 'internals@lists.php.net' > Subject: RE: [PHP-DEV] Patch for opcode caches > > > Hi Marcus, > > > > -----Original Message----- > > > From: Marcus Boerger [mailto:helly@php.net] > > > Sent: Sunday, March 09, 2008 3:00 PM > > > To: Dmitry Stogov > > > Cc: Derick Rethans; internals Mailing List; Andi Gutmans; Stas > > > Malyshev; phpxcache > > > Subject: Re: [PHP-DEV] Patch for opcode caches > > > > > > Hello Dmitry, > > > > > > please don't apply. The patch looks rather rough and untested (see > > > below). > > It was tested at least with Zend products and xcache. > > > > Also I really disagree to making the engine even > > > more complex > > > and adding > > > even more different behavior ways. That way we just introduce more > > > errors > > > as we cannot test the engine in all its modes. > > It is not a good argument to hide this from APC, eAccelerator > and xcache. > > > > We simply do not have > > > the > > > infrastructure for that. And that means we will add even > more bugs. > > You will able to test it with "make test" and new versions of > APC/eAccelerator/XCache ... At least I tested it in this way > without problems > > > > Further more I am missgin a description what you really > do here and > > > why we need to do that. > > This is the description of the problem > > http://www.mail-archive.com/internals@lists.php.net/msg32601.html > It is well known to all opcode cache weiters, but it is not > possible to solve it 100% correctly without ZE modification > > > > Ok Opcode caches have issues but so far all > > > attempts to do > > > somethign about that have been blocked. Now this one > > apparently has a > > > new > > > option. But as far as I can tell it simply allows to change the > > > compiler to an opcode friendly order. If that is all what it does > > > than we should simply > > > drop all the options and do it anyway without flags. > > As I remember opcode cache was planned as a part of PHP-6, > probably there we will able to follow your suggestion, but > for now I don't see any reason to make PHP slowdown (without caches). > > Thank you for patch review > > 1) CG(extended_info) was a boolean falg, CG(compiler_options) > is a bitset one bit of which represents old CG(extended_info) > > 2) Changes related to ZEND_ACC_IMPLEMENT_INTERFACES fix > unnecessary zend_verify_abstarct_class() calls. (For some > reason it might be called twice for the same class) > > 3) ZEND_COMPILE_OPTION_... is to long, but I can accept this > and of couse I'll add comments as you requested. > > 4) zend_vm_opcodes.h is autogenerated file, zend_vm_gen.php > chenged the indents :) > > 5) I'll recheck the -e option. Probably I missied something there. > > Thanks. Dmitry. > > > > unnoticed. > > > > > > marcus > > > > > > Friday, March 7, 2008, 12:36:58 PM, you wrote: > > > > > > > Index: Zend/zend.c > > > > > > =================================================================== > > > > RCS file: /repository/ZendEngine2/zend.c,v > > > > retrieving revision 1.308.2.12.2.35.2.9 > > > > diff -u -p -d -r1.308.2.12.2.35.2.9 zend.c > > > > --- Zend/zend.c 5 Mar 2008 13:34:12 -0000 > > 1.308.2.12.2.35.2.9 > > > > +++ Zend/zend.c 7 Mar 2008 11:34:14 -0000 > > > > @@ -463,7 +463,7 @@ static void zend_set_default_compile_tim > > > > CG(asp_tags) = asp_tags_default; > > > > CG(short_tags) = short_tags_default; > > > > CG(allow_call_time_pass_reference) = > ct_pass_ref_default; > > > > - CG(extended_info) = extended_info_default; > > > > + CG(compiler_options) = compiler_options_default; > > > > } > > > > /* }}} */ > > > > > > > > > > Why rename this variable? It still is an exetended > > information in the > > > compiler globals. It's full name would be > > > compiler_globals_extended_info' versus > > > 'compiler_globals_compiler_options'. > > > > > > > Index: Zend/zend_compile.c > > > > > > =================================================================== > > > > RCS file: /repository/ZendEngine2/zend_compile.c,v > > > > retrieving revision 1.647.2.27.2.41.2.46 > > > > diff -u -p -d -r1.647.2.27.2.41.2.46 zend_compile.c > > > > --- Zend/zend_compile.c 3 Mar 2008 15:07:04 -0000 > > > 1.647.2.27.2.41.2.46 > > > > +++ Zend/zend_compile.c 7 Mar 2008 11:34:15 -0000 > > > [...] > > > > @@ -2588,7 +2577,7 @@ ZEND_API void zend_do_inheritance(zend_c > > > > > > > > if (ce->ce_flags & > > ZEND_ACC_IMPLICIT_ABSTRACT_CLASS && ce- > > > >type == ZEND_INTERNAL_CLASS) { > > > > ce->ce_flags |= > ZEND_ACC_EXPLICIT_ABSTRACT_CLASS; > > > > - } else { > > > > + } else if (!(ce->ce_flags & > > ZEND_ACC_IMPLEMENT_INTERFACES)) > > > > + { > > > > zend_verify_abstract_class(ce TSRMLS_CC); > > > > } > > > > } > > > > @@ -2700,7 +2689,7 @@ ZEND_API zend_class_entry *do_bind_class > > > > } > > > > return NULL; > > > > } else { > > > > - if (!(ce->ce_flags & ZEND_ACC_INTERFACE)) { > > > > + if (!(ce->ce_flags & > > > > (ZEND_ACC_INTERFACE|ZEND_ACC_IMPLEMENT_INTERFACES))) { > > > > zend_verify_abstract_class(ce > TSRMLS_CC); > > > > } > > > > return ce; > > > > > > This looks like a change of behavior to the untrained eye. > > > > > > [...] > > > > @@ -3238,54 +3235,36 @@ void zend_do_end_class_declaration(znode > > > > > > > > ce->line_end = zend_get_compiled_lineno(TSRMLS_C); > > > > > > > > - if (!(ce->ce_flags & > > > > (ZEND_ACC_INTERFACE|ZEND_ACC_EXPLICIT_ABSTRACT_CLASS)) > > > > - && ((parent_token->op_type != IS_UNUSED) || (ce- > > > >num_interfaces > 0))) { > > > > + if (!(ce->ce_flags & > > > > (ZEND_ACC_INTERFACE|ZEND_ACC_EXPLICIT_ABSTRACT_CLASS))) { > > > > zend_verify_abstract_class(ce TSRMLS_CC); > > > > - if (ce->parent || ce->num_interfaces) { > > > > + if (ce->ce_flags & > > ZEND_ACC_IMPLEMENT_INTERFACES) { > > > > do_verify_abstract_class(TSRMLS_C); > > > > } > > > > } > > > > - /* Inherit interfaces; reset number to zero, we > > need it for > > > above check and > > > > - * will restore it during actual implementation. */ > > > > - if (ce->num_interfaces > 0) { > > > > - ce->interfaces = NULL; > > > > - ce->num_interfaces = 0; > > > > - } > > > > CG(active_class_entry) = NULL; > > > > } > > > > > > Again looks like change of behavior. > > > > > > [...] > > > > > > > Index: Zend/zend_compile.h > > > > > > =================================================================== > > > > RCS file: /repository/ZendEngine2/zend_compile.h,v > > > > retrieving revision 1.316.2.8.2.12.2.14 > > > > diff -u -p -d -r1.316.2.8.2.12.2.14 zend_compile.h > > > > --- Zend/zend_compile.h 12 Feb 2008 00:20:33 -0000 > > > 1.316.2.8.2.12.2.14 > > > > +++ Zend/zend_compile.h 7 Mar 2008 11:34:15 -0000 > > > > @@ -143,6 +143,10 @@ typedef struct _zend_try_catch_element { > > > > /* deprecation flag */ > > > > #define ZEND_ACC_DEPRECATED 0x40000 > > > > > > > > +/* class implement interface(s) flag */ > > > > > > 'Class implements interface(s) flag' Not the additional s. > > Either way, > > > this comment is completely useless. The name of the damn > flag tells > > > me that it has something to do with classes implementing > interfaces. > > > A comment is only uiseful if it adds information. And yes > > names should > > > be > > > clear. > > > > > > > +#define ZEND_ACC_IMPLEMENT_INTERFACES 0x80000 > > > > + > > > > + > > > > char *zend_visibility_string(zend_uint fn_flags); > > > > > > > > > > > > + > > > > +#define ZEND_COMPILE_EXTENDED_INFO > > > (1<<0) > > > > +#define ZEND_COMPILE_HANDLE_OP_ARRAY (1<<1) > > > > +#define ZEND_COMPILE_IGNORE_INTERNAL_FUNCTIONS (1<<2) > > > > +#define ZEND_COMPILE_IGNORE_INTERNAL_CLASSES (1<<3) > > > > +#define ZEND_COMPILE_DELAYED_BINDING (1<<4) > > > > + > > > > +#define ZEND_COMPILE_DEFAULT > > > ZEND_COMPILE_HANDLE_OP_ARRAY > > > > +#define ZEND_COMPILE_DEFAULT_FOR_EVAL 0 > > > > + > > > > > > The above are all possible options. So why not name them > > > 'ZEND_COMPILE_OPTION_*' and why no comments here? > > > > > > [...] > > > > Index: Zend/zend_vm_opcodes.h > > > > > > =================================================================== > > > > RCS file: /repository/ZendEngine2/zend_vm_opcodes.h,v > > > > retrieving revision 1.42.2.17.2.1.2.4 > > > > diff -u -p -d -r1.42.2.17.2.1.2.4 zend_vm_opcodes.h > > > > --- Zend/zend_vm_opcodes.h 31 Dec 2007 07:17:06 -0000 > > > 1.42.2.17.2.1.2.4 > > > > +++ Zend/zend_vm_opcodes.h 7 Mar 2008 11:34:26 -0000 > > > > @@ -18,135 +18,136 @@ > > > > > > > > > +---------------------------------------------------------------- > > > ------+ > > > > */ > > > > > > > > -#define ZEND_NOP 0 > > > [...] > > > > +#define ZEND_NOP 0 > > > Please do not change the indent here or create the patch > > with -uwp as > > > otherwise one cannot tell where you added new stuff. > > > > Index: sapi/cgi/cgi_main.c > > > > > > =================================================================== > > > > RCS file: /repository/php-src/sapi/cgi/cgi_main.c,v > > > > retrieving revision 1.267.2.15.2.50.2.13 > > > > diff -u -p -d -r1.267.2.15.2.50.2.13 cgi_main.c > > > > --- sapi/cgi/cgi_main.c 28 Feb 2008 00:51:56 -0000 > > > 1.267.2.15.2.50.2.13 > > > > +++ sapi/cgi/cgi_main.c 7 Mar 2008 11:34:29 -0000 > > > > @@ -1691,7 +1691,7 @@ consult the installation file that came > > > > break; > > > > > > > > case 'e': > > /* enable > > > extended info output */ > > > > - > > > CG(extended_info) = 1; > > > > + > > > > CG(compiler_options) |= ZEND_COMPILE_EXTENDED_INFO; > > > > break; > > > > > > > > case > 'f': /* parse > > > file */ > > > > > > This does not work. > > > > > > > Index: sapi/cli/php_cli.c > > > > > > =================================================================== > > > > RCS file: /repository/php-src/sapi/cli/php_cli.c,v > > > > retrieving revision 1.129.2.13.2.22.2.4 > > > > diff -u -p -d -r1.129.2.13.2.22.2.4 php_cli.c > > > > --- sapi/cli/php_cli.c 3 Feb 2008 17:49:46 -0000 > > > 1.129.2.13.2.22.2.4 > > > > +++ sapi/cli/php_cli.c 7 Mar 2008 11:34:29 -0000 > > > > @@ -821,7 +821,7 @@ int main(int argc, char *argv[]) > > > > break; > > > > > > > > case 'e': /* enable extended info > > output */ > > > > - CG(extended_info) = 1; > > > > + CG(compiler_options) |= > > > ZEND_COMPILE_EXTENDED_INFO; > > > > break; > > > > > > > > case 'F': > > > > > > Does not work either. > > > > > > > Index: sapi/milter/php_milter.c > > > > > > =================================================================== > > > > RCS file: /repository/php-src/sapi/milter/php_milter.c,v > > > > retrieving revision 1.14.2.2.2.3.2.2 > > > > diff -u -p -d -r1.14.2.2.2.3.2.2 php_milter.c > > > > --- sapi/milter/php_milter.c 31 Dec 2007 07:17:18 -0000 > > > 1.14.2.2.2.3.2.2 > > > > +++ sapi/milter/php_milter.c 7 Mar 2008 11:34:29 -0000 > > > > @@ -1033,7 +1033,7 @@ int main(int argc, char *argv[]) > > > > break; > > > > > > > > case 'e': /* enable extended info > > output */ > > > > - CG(extended_info) = 1; > > > > + CG(compiler_options) |= > > > ZEND_COMPILE_EXTENDED_INFO; > > > > break; > > > > > > > > case 'f': /* parse file */ > > > > > > And this does also not work. > > > > > > The reason is that you did not update the table that allows those > > > additions. Given that fact I doubt that you tested what > you did and > > > hence this is just a quick shot at something I fail to get. > > > > > > Best regards, > > > Marcus > > > > >