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