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 > >