Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:36067 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 1281 invoked from network); 9 Mar 2008 21:59:56 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 9 Mar 2008 21:59:56 -0000 Authentication-Results: pb1.pair.com smtp.mail=helly@php.net; spf=unknown; sender-id=unknown Authentication-Results: pb1.pair.com header.from=helly@php.net; sender-id=unknown Received-SPF: unknown (pb1.pair.com: domain php.net does not designate 85.214.94.56 as permitted sender) X-PHP-List-Original-Sender: helly@php.net X-Host-Fingerprint: 85.214.94.56 aixcept.net Linux 2.6 Received: from [85.214.94.56] ([85.214.94.56:37547] helo=h1149922.serverkompetenz.net) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id 20/0B-43615-BDD54D74 for ; Sun, 09 Mar 2008 16:59:55 -0500 Received: from localhost (h1149922.serverkompetenz.net [85.214.94.56]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by h1149922.serverkompetenz.net (Postfix) with ESMTP id F3AF911F244; Sun, 9 Mar 2008 22:59:50 +0100 (CET) Date: Sun, 9 Mar 2008 22:59:50 +0100 Reply-To: Marcus Boerger X-Priority: 3 (Normal) Message-ID: <82388890.20080309225950@marcus-boerger.de> To: "Dmitry Stogov" CC: "Derick Rethans" , "internals Mailing List" , "Andi Gutmans" , "Stas Malyshev" , "phpxcache" In-Reply-To: <06B0D32C7A96544490D18AF653D6BDE50261887F@il-ex1.zend.net> References: <06B0D32C7A96544490D18AF653D6BDE50261887F@il-ex1.zend.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: 8bit Subject: Re: [PHP-DEV] Patch for opcode caches From: helly@php.net (Marcus Boerger) Hello Dmitry, please don't apply. The patch looks rather rough and untested (see below). 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. We simply do not have the infrastructure for that. And that means we will add even more bugs. Further more I am missgin a description what you really do here and why we need to do that. 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. 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