Electronics & Programming

develissimo

Open Source electronics development and programming

  • You are not logged in.
  • Root
  • » PHP
  • » [PHP-DEV] Patch for opcode caches [RSS Feed]

#1 March 7, 2008 10:17:41

Dmitry S.
Registered: 2009-11-02
Reputation: +  0  -
Profile   Send e-mail  

[PHP-DEV] Patch for opcode caches


Hi,

The attached patch for PHP_5_3 is going to provide general solution to
control some aspects of PHP compilation. It is absolutely necessary for
all opcode caches to make cached scripts work exactly in the same way as
non-cached.
The problem occurs because early binding of inherited classes is not
possible for cached scripts. As result inheritance is handled in
run-time and classes might be created in wrong order. Most opcode caches
do some tricks to resolve this issue, but all of them fail to do it
right way.

The patch introduces CG(compiler_options), which is a set of compiler
flags that can be used by some extensions as opcode caches and
debuggers. Such flags change the process of compilation a bit.

For example to solve inherence early binding problem the opcode cache
need to compile file with additional flags

extension_compile_file() {
...
orig_compiler_options = CG(compiler_options);
CG(compiler_options) |= ZEND_COMPILE_IGNORE_INTERNAL_CLASSES |
ZEND_COMPILE_DELAYED_BINDING;
orig_compile_file();
CG(compiler_options) = orig_compiler_options;
...
}

And then during restoration from cache call
zend_do_delayed_early_binding().
This way even with cache all classes will be created exactly in the same
order.

The same CG(compiler_options) might be used to change other aspects of
compilation in the future.

The patch also optimizes ZEND_FETCH_CLASS+ZEND_ADD_INTERFACE opcodes
into single ZEND_ADD_INTERFACE, and makes verification of abstract
classes with interfaces only once.

I'm going to commit the patch on Monday or Tuesday.

Thanks. Dmitry.--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit:http://www.php.net/unsub.php

Offline

#2 March 7, 2008 10:31:33

Derick R.
Registered: 2009-11-02
Reputation: +  0  -
Profile   Send e-mail  

[PHP-DEV] Patch for opcode caches


On Fri, 7 Mar 2008, Dmitry Stogov wrote:

> The attached patch for PHP_5_3 is going to provide general solution to
> control some aspects of PHP compilation. It is absolutely necessary for
> all opcode caches to make cached scripts work exactly in the same way as
> non-cached.

There's no attached patch.

regards,
Derick
--
Derick Rethanshttp://derickrethans.nl|http://ezcomponents.org|http://xdebug.org--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit:http://www.php.net/unsub.php

Offline

#3 March 8, 2008 09:12:12

p.
Registered: 2009-11-02
Reputation: +  0  -
Profile   Send e-mail  

[PHP-DEV] Patch for opcode caches


checked and it works, all test cases that pass without XCache now pass
with XCache too. i'll commit after your commit in ZendEngine

thanks

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit:http://www.php.net/unsub.php

Offline

#4 March 9, 2008 23:01:05

Marcus B.
Registered: 2009-11-02
Reputation: +  0  -
Profile   Send e-mail  

[PHP-DEV] Patch for opcode caches


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


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit:http://www.php.net/unsub.php

Offline

#5 March 10, 2008 02:29:49

p.
Registered: 2009-11-02
Reputation: +  0  -
Profile   Send e-mail  

[PHP-DEV] Patch for opcode caches


On Mon, Mar 10, 2008 at 5:59 AM, Marcus Boerger <> wrote:
> 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.
>
just a summary of the patch for those who don't have time to read it:
ZEND_COMPILE_EXTENDED_INFO is a new way for the extended_info = 1, no
logic changed except the api, only a few modules is affected, and yes
it can be changed back to avoid breaking 3rd modules at source level

the issues relatived to ZEND_COMPILE_DELAYED_BINDING /
ZEND_COMPILE_IGNORE_INTERNAL_* can be fixed without those flags.
but when we have the flag off the old implementation is used which is
more optimized than the new opcode/encoder friendly implementation.

correct me if i'm wrong. i'll leave the discussion to you guys, just
move it fast before it's too late. thanks

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit:http://www.php.net/unsub.php

Offline

#6 March 10, 2008 12:53:59

Robin F.
Registered: 2009-11-02
Reputation: +  0  -
Profile   Send e-mail  

[PHP-DEV] Patch for opcode caches


Hi phpxcache,

On 08/03/2008, phpxcache <> wrote:
> all test cases that pass without XCache now pass
> with XCache too.

Are these tests that are already in the cvs repository? If not, it
would be great to get them in. Sounds like they could be useful
whether or not the patch gets applied. They might also help illustrate
the patch.

Regards,
Robin

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit:http://www.php.net/unsub.php

Offline

#7 March 10, 2008 13:48:58

Dmitry S.
Registered: 2009-11-02
Reputation: +  0  -
Profile   Send e-mail  

[PHP-DEV] Patch for opcode caches


Hi Marcus,

> > -----Original Message-----
> > From: Marcus Boerger
> > Sent: Sunday, March 09, 2008 3:00 PM
> > To: Dmitry Stogov
> > Cc: Derick Rethans; internals Mailing List; Andi Gutmans; Stas
> > Malyshev; phpxcache
> > Subject: Re: 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 problemhttp://www.mail-archive.com/intern***@*ists.php.net/msg32601.htmlIt 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
>
>

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit:http://www.php.net/unsub.php

Offline

#8 March 10, 2008 17:31:48

Stanislav M.
Registered: 2009-11-02
Reputation: +  0  -
Profile   Send e-mail  

[PHP-DEV] Patch for opcode caches


Hi!This does not work.
Does not work either.
And this does also not work.

The reason is that you did not update the table that allows thoseWhich table are you referring to?
--
Stanislav Malyshev, Zend Software Architect
http://www.zend.com/(408)253-8829 MSN:

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit:http://www.php.net/unsub.php

Offline

#9 March 11, 2008 09:21:31

Dmitry S.
Registered: 2009-11-02
Reputation: +  0  -
Profile   Send e-mail  

[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';
> 'intern***@*ists.php.net'
> Subject: RE: Patch for opcode caches
>
>
> Hi Marcus,
>
> > > -----Original Message-----
> > > From: Marcus Boerger
> > > Sent: Sunday, March 09, 2008 3:00 PM
> > > To: Dmitry Stogov
> > > Cc: Derick Rethans; internals Mailing List; Andi Gutmans; Stas
> > > Malyshev; phpxcache
> > > Subject: Re: 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/intern***@*ists.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
> >
> >
>

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit:http://www.php.net/unsub.php

Offline

#10 March 13, 2008 19:28:04

Marcus B.
Registered: 2009-11-02
Reputation: +  0  -
Profile   Send e-mail  

[PHP-DEV] Patch for opcode caches


Hello Dmitry,

Monday, March 10, 2008, 1:48:05 PM, you wrote:

> Hi Marcus,

>> > -----Original Message-----
>> > From: Marcus Boerger
>> > Sent: Sunday, March 09, 2008 3:00 PM
>> > To: Dmitry Stogov
>> > Cc: Derick Rethans; internals Mailing List; Andi Gutmans; Stas
>> > Malyshev; phpxcache
>> > Subject: Re: 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.

Good, I am just worried that we introduce different behavior. We should
just have one execution mode. Your change is better now having a more
detailed view on what it does. But I think we should not have the old model
as well.

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

But the majority of people cannot unless we bundle one of those. So all we
normal people get is an increased ability of errors.

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

good catch then

> 3) ZEND_COMPILE_OPTION_... is to long, but I can accept this and of
> couse I'll add comments as you requested.

good :-)

> 4) zend_vm_opcodes.h is autogenerated file, zend_vm_gen.php chenged the
> indents :)

ok

> 5) I'll recheck the -e option. Probably I missied something there.

I did not see -e in the list of allowed flags. So it cannot work.

Best regards,
Marcus


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit:http://www.php.net/unsub.php

Offline

  • Root
  • » PHP
  • » [PHP-DEV] Patch for opcode caches [RSS Feed]

Board footer

Moderator control

Enjoy the 16th of December
PoweredBy

The Forums are managed by develissimo stuff members, if you find any issues or misplaced content please help us to fix it. Thank you! Tell us via Contact Options
Leave a Message
Welcome to Develissimo Live Support