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 cachesHi 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 cachesHello 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 problemsFurther 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
internals@lists.php.net/msg32601.html" rel="nofollow" target="_blank">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 modificationOk 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
CG(extended_info) was a boolean falg, CG(compiler_options)
is a bitset one bit of which represents old CG(extended_info)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)ZEND_COMPILE_OPTION_... is to long, but I can accept this
and of couse I'll add comments as you requested.zend_vm_opcodes.h is autogenerated file, zend_vm_gen.php
chenged the indents :)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_cif (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, weneed 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 infooutput */
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 infooutput */
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