Re: [PHP-DEV] Re: #20993 [Ver]:Elementvaluechangeswithoutasking

[Andi Gutmans]

I meant a big warning should be in place :)

Andi

At 08:18 PM 12/21/2002 +0200, Andi Gutmans wrote:

At 07:13 PM 12/21/2002 +0100, Melvyn Sopacua wrote:

At 18:24 21-12-2002, Andi Gutmans wrote:


Yes I object. Think of references as a "symbolic link" to the same 
place. Now you have such a reference in an array which you assign to an 
additional variable. Now two variables contain copies of an array but 
somewhere inside you have two copies of the symbolic link. Changing one 
will change the other. In many ways it makes sense, and even in cases 
where it's a bit weird and where it seems wrong, I think we should live 
with it because I don't like having to check the whole array each time. 
If you have deeply nested arrays it's really slow and doesn't make that 
much sense.

Ok - so there's no way to detect this at the lexer level?


Nope.


I get the symlink anology, but IMO it doesn't apply here. A copy of the 
array is passed to the function - not a reference.

Yeah but one of those values inside the array is a reference. In both the 
original and the copy of the array it's the same reference.

If you wanna keep the anology, then this resembles symlinking a file, 
inside a directory and changing a *copy* of the directory, changes the 
original directory.

So - IF it can't be warned about - then we need a big fat warning in the 
docs.

Definitely I think a big warning is in place.

Andi




Andi

At 05:24 PM 12/21/2002 +0100, Melvyn Sopacua wrote:

At 17:16 18-12-2002, Moriyoshi Koizumi wrote:


Melvyn Sopacua <[EMAIL PROTECTED]> wrote:

--snip
> > > OK so that's a deep copy. As much as I understand the 
motivation I don't
> > > think this should be done. It'll slow down lots of things in 
PHP. I think
> > > this should be solved by documentation.
> >
> >Yes, according to my trivial benchmark, my patch puts a considerable
> >weight on the ZendEngine, to run twice as slowly as the current 
runtime in
> >the worst case. Seems no way, but I suppose it also sounds a 
reasonable
> >penalty of using references.
>
> Actually - the natural 'feeling' with references is speed increases 
- not
> slowdowns,
> since one expects a 'pointer', rather than a copy.
>
> Is there a way to warn when such a refstatement is detected, without
> causing slowdowns?

Then try the new patch. It prints out notices in such cases.

Attached is a slightly revised version against PHP_4_3 branch - just an 
'english'
fix.

If there are no objections, can somebody commit it?

I'll fix the test accordingly.


With kind regards,

Melvyn Sopacua



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


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



With kind regards,

Melvyn Sopacua




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



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

Re: [PHP-DEV] Re: #20993 [Ver]:Elementvaluechangeswithoutasking

[Andi Gutmans]

At 07:13 PM 12/21/2002 +0100, Melvyn Sopacua wrote:

At 18:24 21-12-2002, Andi Gutmans wrote:


Yes I object. Think of references as a "symbolic link" to the same place. 
Now you have such a reference in an array which you assign to an 
additional variable. Now two variables contain copies of an array but 
somewhere inside you have two copies of the symbolic link. Changing one 
will change the other. In many ways it makes sense, and even in cases 
where it's a bit weird and where it seems wrong, I think we should live 
with it because I don't like having to check the whole array each time. 
If you have deeply nested arrays it's really slow and doesn't make that 
much sense.

Ok - so there's no way to detect this at the lexer level?


Nope.


I get the symlink anology, but IMO it doesn't apply here. A copy of the 
array is passed to the function - not a reference.

Yeah but one of those values inside the array is a reference. In both the 
original and the copy of the array it's the same reference.

If you wanna keep the anology, then this resembles symlinking a file, 
inside a directory and changing a *copy* of the directory, changes the 
original directory.

So - IF it can't be warned about - then we need a big fat warning in the docs.

Definitely I think a big warning is in place.

Andi




Andi

At 05:24 PM 12/21/2002 +0100, Melvyn Sopacua wrote:

At 17:16 18-12-2002, Moriyoshi Koizumi wrote:


Melvyn Sopacua <[EMAIL PROTECTED]> wrote:

--snip
> > > OK so that's a deep copy. As much as I understand the motivation 
I don't
> > > think this should be done. It'll slow down lots of things in 
PHP. I think
> > > this should be solved by documentation.
> >
> >Yes, according to my trivial benchmark, my patch puts a considerable
> >weight on the ZendEngine, to run twice as slowly as the current 
runtime in
> >the worst case. Seems no way, but I suppose it also sounds a reasonable
> >penalty of using references.
>
> Actually - the natural 'feeling' with references is speed increases 
- not
> slowdowns,
> since one expects a 'pointer', rather than a copy.
>
> Is there a way to warn when such a refstatement is detected, without
> causing slowdowns?

Then try the new patch. It prints out notices in such cases.

Attached is a slightly revised version against PHP_4_3 branch - just an 
'english'
fix.

If there are no objections, can somebody commit it?

I'll fix the test accordingly.


With kind regards,

Melvyn Sopacua



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


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




With kind regards,

Melvyn Sopacua




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

Re: [PHP-DEV] Re: #20993 [Ver]:Elementvaluechangeswithoutasking

[Melvyn Sopacua]

At 18:24 21-12-2002, Andi Gutmans wrote:


Yes I object. Think of references as a "symbolic link" to the same place. 
Now you have such a reference in an array which you assign to an 
additional variable. Now two variables contain copies of an array but 
somewhere inside you have two copies of the symbolic link. Changing one 
will change the other. In many ways it makes sense, and even in cases 
where it's a bit weird and where it seems wrong, I think we should live 
with it because I don't like having to check the whole array each time. If 
you have deeply nested arrays it's really slow and doesn't make that much 
sense.

Ok - so there's no way to detect this at the lexer level?

I get the symlink anology, but IMO it doesn't apply here. A copy of the 
array is passed to the function - not a reference.

If you wanna keep the anology, then this resembles symlinking a file, 
inside a directory and changing a *copy* of the directory, changes the 
original directory.

So - IF it can't be warned about - then we need a big fat warning in the docs.



Andi

At 05:24 PM 12/21/2002 +0100, Melvyn Sopacua wrote:

At 17:16 18-12-2002, Moriyoshi Koizumi wrote:


Melvyn Sopacua <[EMAIL PROTECTED]> wrote:

--snip
> > > OK so that's a deep copy. As much as I understand the motivation 
I don't
> > > think this should be done. It'll slow down lots of things in PHP. 
I think
> > > this should be solved by documentation.
> >
> >Yes, according to my trivial benchmark, my patch puts a considerable
> >weight on the ZendEngine, to run twice as slowly as the current 
runtime in
> >the worst case. Seems no way, but I suppose it also sounds a reasonable
> >penalty of using references.
>
> Actually - the natural 'feeling' with references is speed increases - not
> slowdowns,
> since one expects a 'pointer', rather than a copy.
>
> Is there a way to warn when such a refstatement is detected, without
> causing slowdowns?

Then try the new patch. It prints out notices in such cases.

Attached is a slightly revised version against PHP_4_3 branch - just an 
'english'
fix.

If there are no objections, can somebody commit it?

I'll fix the test accordingly.


With kind regards,

Melvyn Sopacua



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


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





With kind regards,

Melvyn Sopacua



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

Re: [PHP-DEV] Re: #20993 [Ver]:Elementvaluechangeswithoutasking

[Andi Gutmans]

Yes I object. Think of references as a "symbolic link" to the same place. 
Now you have such a reference in an array which you assign to an additional 
variable. Now two variables contain copies of an array but somewhere inside 
you have two copies of the symbolic link. Changing one will change the 
other. In many ways it makes sense, and even in cases where it's a bit 
weird and where it seems wrong, I think we should live with it because I 
don't like having to check the whole array each time. If you have deeply 
nested arrays it's really slow and doesn't make that much sense.

Andi

At 05:24 PM 12/21/2002 +0100, Melvyn Sopacua wrote:
At 17:16 18-12-2002, Moriyoshi Koizumi wrote:


Melvyn Sopacua <[EMAIL PROTECTED]> wrote:

--snip
> > > OK so that's a deep copy. As much as I understand the motivation I 
don't
> > > think this should be done. It'll slow down lots of things in PHP. 
I think
> > > this should be solved by documentation.
> >
> >Yes, according to my trivial benchmark, my patch puts a considerable
> >weight on the ZendEngine, to run twice as slowly as the current 
runtime in
> >the worst case. Seems no way, but I suppose it also sounds a reasonable
> >penalty of using references.
>
> Actually - the natural 'feeling' with references is speed increases - not
> slowdowns,
> since one expects a 'pointer', rather than a copy.
>
> Is there a way to warn when such a refstatement is detected, without
> causing slowdowns?

Then try the new patch. It prints out notices in such cases.

Attached is a slightly revised version against PHP_4_3 branch - just an 
'english'
fix.

If there are no objections, can somebody commit it?

I'll fix the test accordingly.


With kind regards,

Melvyn Sopacua



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


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

Re: [PHP-DEV] Re: #20993 [Ver]:Elementvaluechangeswithoutasking

[Melvyn Sopacua]

At 17:16 18-12-2002, Moriyoshi Koizumi wrote:


Melvyn Sopacua <[EMAIL PROTECTED]> wrote:

--snip
> > > OK so that's a deep copy. As much as I understand the motivation I 
don't
> > > think this should be done. It'll slow down lots of things in PHP. I 
think
> > > this should be solved by documentation.
> >
> >Yes, according to my trivial benchmark, my patch puts a considerable
> >weight on the ZendEngine, to run twice as slowly as the current runtime in
> >the worst case. Seems no way, but I suppose it also sounds a reasonable
> >penalty of using references.
>
> Actually - the natural 'feeling' with references is speed increases - not
> slowdowns,
> since one expects a 'pointer', rather than a copy.
>
> Is there a way to warn when such a refstatement is detected, without
> causing slowdowns?

Then try the new patch. It prints out notices in such cases.

Attached is a slightly revised version against PHP_4_3 branch - just an 
'english'
fix.

If there are no objections, can somebody commit it?

I'll fix the test accordingly.


With kind regards,

Melvyn Sopacua
Index: Zend/zend_execute.c
===
RCS file: /repository/Zend/zend_execute.c,v
retrieving revision 1.316.2.2
diff -u -r1.316.2.2 zend_execute.c
--- Zend/zend_execute.c 17 Nov 2002 22:00:32 -  1.316.2.2
+++ Zend/zend_execute.c 21 Dec 2002 15:42:06 -
@@ -65,6 +65,7 @@
 static void zend_extension_statement_handler(zend_extension *extension, zend_op_array 
*op_array TSRMLS_DC);
 static void zend_extension_fcall_begin_handler(zend_extension *extension, 
zend_op_array *op_array TSRMLS_DC);
 static void zend_extension_fcall_end_handler(zend_extension *extension, zend_op_array 
*op_array TSRMLS_DC);
+static int check_array_contains_ref(zval **ppz);
 
 #define RETURN_VALUE_USED(opline) (!((opline)->result.u.EA.type & EXT_TYPE_UNUSED))
 
@@ -1816,6 +1817,9 @@
} else if (PZVAL_IS_REF(*param)) {

zend_assign_to_variable_reference(NULL, get_zval_ptr_ptr(&EX(opline)->result, EX(Ts), 
BP_VAR_W), param, NULL TSRMLS_CC);
} else {
+   if (check_array_contains_ref(param)) {
+   zend_error(E_NOTICE, "Array 
+passed to %s() (argument #%d) contains referenced element(s) which may result in 
+unexpected behavior.", get_active_function_name(TSRMLS_C), 
+EX(opline)->op1.u.constant.value.lval);
+   }
zend_assign_to_variable(NULL, 
&EX(opline)->result, NULL, *param, IS_VAR, EX(Ts) TSRMLS_CC);
}
}
@@ -2480,4 +2484,23 @@
}
}
zend_error(E_ERROR, "Arrived at end of main loop which shouldn't happen");
+}
+
+static int _zval_ref_check(zval **p, void *flag)
+{
+   if ((*p)->is_ref) {
+   *(int *)flag = 1;
+   } else {
+   *(int *)flag = check_array_contains_ref(p);
+   }
+   return ZEND_HASH_APPLY_KEEP;
+}
+
+static int check_array_contains_ref(zval **ppz)
+{
+   int flag = 0;
+   if ((*ppz)->type == IS_ARRAY) {
+   zend_hash_apply_with_argument((*ppz)->value.ht, (apply_func_arg_t) 
+_zval_ref_check, (void *)&flag TSRMLS_CC);
+   } 
+   return flag;
 }

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

Re: [PHP-DEV] Re: #20993 [Ver]:Elementvaluechangeswithoutasking

[Moriyoshi Koizumi]

Melvyn Sopacua <[EMAIL PROTECTED]> wrote:

--snip
> > > OK so that's a deep copy. As much as I understand the motivation I don't
> > > think this should be done. It'll slow down lots of things in PHP. I think
> > > this should be solved by documentation.
> >
> >Yes, according to my trivial benchmark, my patch puts a considerable
> >weight on the ZendEngine, to run twice as slowly as the current runtime in
> >the worst case. Seems no way, but I suppose it also sounds a reasonable
> >penalty of using references.
> 
> Actually - the natural 'feeling' with references is speed increases - not 
> slowdowns,
> since one expects a 'pointer', rather than a copy.
> 
> Is there a way to warn when such a refstatement is detected, without 
> causing slowdowns?

Then try the new patch. It prints out notices in such cases.

--
Benchmark results (the script used for this test is the same as the one 
attached in my previous mail)

[Unmodified]
1: 0.166993
2: 0.099101
3: 0.219380
4: 0.094828

[After applying the new patch]
1: 0.189953
2: 0.099915
3: 0.238101
4: 0.095103
--

Moriyoshi

> With kind regards,
> 
> Melvyn Sopacua
> 
> 
> 
> -- 
> PHP Development Mailing List 
> To unsubscribe, visit: http://www.php.net/unsub.php
> 

Index: Zend/zend_execute.c
===
RCS file: /repository/Zend/zend_execute.c,v
retrieving revision 1.316.2.2
diff -u -r1.316.2.2 zend_execute.c
--- Zend/zend_execute.c 17 Nov 2002 22:00:32 -  1.316.2.2
+++ Zend/zend_execute.c 18 Dec 2002 16:08:38 -
@@ -65,6 +65,7 @@
 static void zend_extension_statement_handler(zend_extension *extension, zend_op_array 
*op_array TSRMLS_DC);
 static void zend_extension_fcall_begin_handler(zend_extension *extension, 
zend_op_array *op_array TSRMLS_DC);
 static void zend_extension_fcall_end_handler(zend_extension *extension, zend_op_array 
*op_array TSRMLS_DC);
+static int check_array_contains_ref(zval **ppz);
 
 #define RETURN_VALUE_USED(opline) (!((opline)->result.u.EA.type & EXT_TYPE_UNUSED))
 
@@ -1816,6 +1817,9 @@
} else if (PZVAL_IS_REF(*param)) {

zend_assign_to_variable_reference(NULL, get_zval_ptr_ptr(&EX(opline)->result, EX(Ts), 
BP_VAR_W), param, NULL TSRMLS_CC);
} else {
+   if (check_array_contains_ref(param)) {
+   zend_error(E_NOTICE, "Array 
+passed to %s() (argument #%d) contains referenced element(s) which may result in 
+unexpected behaviours", get_active_function_name(TSRMLS_C), 
+EX(opline)->op1.u.constant.value.lval);
+   }
zend_assign_to_variable(NULL, 
&EX(opline)->result, NULL, *param, IS_VAR, EX(Ts) TSRMLS_CC);
}
}
@@ -2480,4 +2484,23 @@
}
}
zend_error(E_ERROR, "Arrived at end of main loop which shouldn't happen");
+}
+
+static int _zval_ref_check(zval **p, void *flag)
+{
+   if ((*p)->is_ref) {
+   *(int *)flag = 1;
+   } else {
+   *(int *)flag = check_array_contains_ref(p);
+   }
+   return ZEND_HASH_APPLY_KEEP;
+}
+
+static int check_array_contains_ref(zval **ppz)
+{
+   int flag = 0;
+   if ((*ppz)->type == IS_ARRAY) {
+   zend_hash_apply_with_argument((*ppz)->value.ht, (apply_func_arg_t) 
+_zval_ref_check, (void *)&flag TSRMLS_CC);
+   } 
+   return flag;
 }

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

Re: [PHP-DEV] Re: #20993 [Ver]: Elementvaluechangeswithoutasking

[Melvyn Sopacua]

At 06:44 18-12-2002, Moriyoshi Koizumi wrote:


Andi Gutmans <[EMAIL PROTECTED]> wrote:

> At 12:49 AM 12/17/2002 +0900, Moriyoshi Koizumi wrote:
> >Andi Gutmans <[EMAIL PROTECTED]> wrote:
> >
> > > I don't understand what you're doing here. Are you actually 
separating on
> > > every assignment and doing a deep copy?
> >
> >What I'm trying to do in my patch can be divided into two phases.
> >
> >In the first phase, it checks whether the array contains any referenced
> >elements (of course it does nothing and return SUCCESS if the passed zval
> >is a non-array value).
> >
> >If such an element exists, then entering the second phase, separates the
> >array zval and duplicates each referenced element while it doesn't make
> >copies, but only increments the refcount for non-referenced elements.
>
> OK so that's a deep copy. As much as I understand the motivation I don't
> think this should be done. It'll slow down lots of things in PHP. I think
> this should be solved by documentation.

Yes, according to my trivial benchmark, my patch puts a considerable
weight on the ZendEngine, to run twice as slowly as the current runtime in
the worst case. Seems no way, but I suppose it also sounds a reasonable
penalty of using references.

Actually - the natural 'feeling' with references is speed increases - not 
slowdowns,
since one expects a 'pointer', rather than a copy.

Is there a way to warn when such a refstatement is detected, without 
causing slowdowns?

With kind regards,

Melvyn Sopacua



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

Re: [PHP-DEV] Re: #20993 [Ver]: Elementvaluechangeswithoutasking

[Moriyoshi Koizumi]

Andi Gutmans <[EMAIL PROTECTED]> wrote:

> At 12:49 AM 12/17/2002 +0900, Moriyoshi Koizumi wrote:
> >Andi Gutmans <[EMAIL PROTECTED]> wrote:
> >
> > > I don't understand what you're doing here. Are you actually separating on
> > > every assignment and doing a deep copy?
> >
> >What I'm trying to do in my patch can be divided into two phases.
> >
> >In the first phase, it checks whether the array contains any referenced
> >elements (of course it does nothing and return SUCCESS if the passed zval
> >is a non-array value).
> >
> >If such an element exists, then entering the second phase, separates the
> >array zval and duplicates each referenced element while it doesn't make
> >copies, but only increments the refcount for non-referenced elements.
> 
> OK so that's a deep copy. As much as I understand the motivation I don't 
> think this should be done. It'll slow down lots of things in PHP. I think 
> this should be solved by documentation.

Yes, according to my trivial benchmark, my patch puts a considerable 
weight on the ZendEngine, to run twice as slowly as the current runtime in 
the worst case. Seems no way, but I suppose it also sounds a reasonable 
penalty of using references.

Moriyoshi

> Andi
> 
> >Moriyoshi
> >
> > > Andi
> > >
> > > At 04:09 PM 12/15/2002 +0900, Moriyoshi Koizumi wrote:
> > > >Oops, the patch was wrong as the runtime occationally segfaults
> > > >in a case like:
> > > >
> > > > > > >   $a = 0;
> > > >   $a = &$a; /* is_ref=1, refcount=1 */
> > > >?>
> > > >
> > > >Attached is the revised patch. Please try it out.
> > > >
> > > >And the result of a tiny benchmark follows:
> > > >
> > > >[Before patching]
> > > >1: 0.263245
> > > >2: 0.142505
> > > >3: 0.328045
> > > >4: 0.137149
> > > >
> > > >[After patching]
> > > >1: 0.273811
> > > >2: 0.141965
> > > >3: 0.699429
> > > >4: 0.137010
> > > >
> > > >Moriyoshi
> > > >
> > > > > My proposal, was based on 2 things: fix or document. I'm sure 
> > Zeev/Andi
> > > > had a
> > > > > good reason not to always separate, and that probably is performance.
> > > > >
> > > > > IF this impacts overall performance very negatively, then maybe the 
> > better
> > > > > choice is to document it.
> > > > >
> > > > > I'll try the patch though.
> > > > >
> > > > >
> > > > > With kind regards,
> > > > >
> > > > > Melvyn Sopacua
> > > > > 
> > > > >
> > > > >
> > > > > --
> > > > > PHP Development Mailing List 
> > > > > To unsubscribe, visit: http://www.php.net/unsub.php
> > > > >
> > > >
> > > >
> > > >--
> > > >PHP Development Mailing List 
> > > >To unsubscribe, visit: http://www.php.net/unsub.php
> > >
> 


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