Everything

php

Fix for bug #25543

php.internals

Moriyoshi Koizumi

22 years ago

Hi,

Attached is a fix for bug #25543 (Error in set_error_handler() definition),
which is caused by disordered scheduling of the garbage collection
(zend_clean_garbage()).

With this patch I'm adding the following two inline functions,
zend_begin_atomic() and zend_end_atomic(), to prevent GC from being
performed at the right time. Calls to those functions are inserted
at the beginning / end of zend_fetch_dimension_address(),
zend_fetch_property_address(), and some other functions of the same kind.

I'll commit these shortly if you don't see any problem.

Regards,

Moriyoshi

Jani Taskinen

22 years ago

#25543 is documentation 'bug', what exactly does your
patch fix? I don't see anything wrong with set_error_handler()..

--Jani

On Fri, 3 Oct 2003, Moriyoshi Koizumi wrote:

r.willenbacher

22 years ago

On October 3, 2003 05:43 am, Jani Taskinen wrote:

>     #25543 is documentation 'bug', what exactly does your
>     patch fix? I don't see anything wrong with set_error_handler()..
>
>     --Jani
>
> On Fri, 3 Oct 2003, Moriyoshi Koizumi wrote:
> >Attached is a fix for bug #25543 (Error in set_error_handler()
> > definition), which is caused by disordered scheduling of the garbage
> > collection (zend_clean_garbage()).

as the patch filenames suggest he meant to fix bug
#25547 (error_handler and array index with function call).

--
ralf willenbacher (bj@ocrana.de)

Moriyoshi Koizumi

22 years ago

Jani Taskinen <sniper@iki.fi> wrote:

>
>     #25543 is documentation 'bug', what exactly does your
>     patch fix? I don't see anything wrong with set_error_handler()..
>

Oops, #25547 is the right number.

Moriyoshi

Andi Gutmans

22 years ago

Hi,

I don't like this patch because already the gc is a very sensitive
mechanism and we're not allowed to collect too much or too little at any
given time. Your patch can easily lead to too much being collected before
previous zval's are freed which can lead to problems (this mechanism was
refined a couple of times due to such problems).
The only real solution I can see is to nuke the garbage completely. I
already have a rough idea of how to do it but it requires quite a lot of work.
BTW, I didn't quite understand the bug report 25543. It doesn't seem to be
very well written (I know it's not you :)

Andi

At 09:32 AM 10/3/2003 +0900, Moriyoshi Koizumi wrote:

Moriyoshi Koizumi

22 years ago

Andi Gutmans <andi@zend.com> wrote:

> Hi,
>
> I don't like this patch because already the gc is a very sensitive
> mechanism and we're not allowed to collect too much or too little at any
> given time. Your patch can easily lead to too much being collected before
> previous zval's are freed which can lead to problems (this mechanism was
> refined a couple of times due to such problems).
> The only real solution I can see is to nuke the garbage completely. I
> already have a rough idea of how to do it but it requires quite a lot of work.
> BTW, I didn't quite understand the bug report 25543. It doesn't seem to be
> very well written (I know it's not you :)

Actually the PR number is 25547 :) Anyway, I don't think my patch is so
harmful because the pointer to a zval (semantically a container of a zval
instance) that has been created at certain znode construction
is supposed not to be destroyed during an atomic operation, by which I
mean a single opcode processing. That's why I named them zend_*_atomic().

Moriyohi

Jani Taskinen

22 years ago

The actual bug report in question is 25547..

--Jani

On Sat, 4 Oct 2003, Andi Gutmans wrote:

Andi Gutmans

22 years ago

At 08:16 AM 10/4/2003 +0900, Moriyoshi Koizumi wrote:

>Andi Gutmans <andi@zend.com> wrote:
>
> > Hi,
> >
> > I don't like this patch because already the gc is a very sensitive
> > mechanism and we're not allowed to collect too much or too little at any
> > given time. Your patch can easily lead to too much being collected before
> > previous zval's are freed which can lead to problems (this mechanism was
> > refined a couple of times due to such problems).
> > The only real solution I can see is to nuke the garbage completely. I
> > already have a rough idea of how to do it but it requires quite a lot
> of work.
> > BTW, I didn't quite understand the bug report 25543. It doesn't seem to be
> > very well written (I know it's not you :)
>
>Actually the PR number is 25547 :) Anyway, I don't think my patch is so
>harmful because the pointer to a zval (semantically a container of a zval
>instance) that has been created at certain znode construction
>is supposed not to be destroyed during an atomic operation, by which I
>mean a single opcode processing. That's why I named them zend_*_atomic().

OK, that explains why the bug report didn't seem to have anything to do
with this problem :)

I disagree. It definitely has potential to be harmful because the garbage
isn't running every opcode like it should. At least that's what I figured
from reading the patch.

Andi

Thies C. Arntzen

22 years ago

On Sat, Oct 04, 2003 at 12:13:12PM +0200, Andi Gutmans wrote:

> At 08:16 AM 10/4/2003 +0900, Moriyoshi Koizumi wrote:
> >Andi Gutmans <andi@zend.com> wrote:
> >
> >> Hi,
> >>
> >> I don't like this patch because already the gc is a very sensitive
> >> mechanism and we're not allowed to collect too much or too little at any
> >> given time. Your patch can easily lead to too much being collected before
> >> previous zval's are freed which can lead to problems (this mechanism was
> >> refined a couple of times due to such problems).
> >> The only real solution I can see is to nuke the garbage completely. I
> >> already have a rough idea of how to do it but it requires quite a lot
> >of work.
> >> BTW, I didn't quite understand the bug report 25543. It doesn't seem to
> >be
> >> very well written (I know it's not you :)
> >
> >Actually the PR number is 25547 :) Anyway, I don't think my patch is so
> >harmful because the pointer to a zval (semantically a container of a zval
> >instance) that has been created at certain znode construction
> >is supposed not to be destroyed during an atomic operation, by which I
> >mean a single opcode processing. That's why I named them zend_*_atomic().
>
> OK, that explains why the bug report didn't seem to have anything to do
> with this problem :)
>
> I disagree. It definitely has potential to be harmful because the garbage
> isn't running every opcode like it should. At least that's what I figured
> from reading the patch.

on a sidenote - i once played around by just adding this
garbage to a linked list and only reclaim it inside emalloc.
that way we don't need to clean it after every op.

this won't fix the 25547 but it would make fixing it less
costy as the gc would not happen as often, and adding code to
it would not slow down the executor.

re,
tc

Moriyoshi Koizumi

22 years ago

"Thies C. Arntzen" <thies@thiesos.org> wrote:

>
>     on a sidenote - i once played around by just adding this
>     garbage to a linked list and only reclaim it inside emalloc.
>     that way we don't need to clean it after every op.
>
>     this won't fix the 25547 but it would make fixing it less
>     costy as the gc would not happen as often, and adding code to
>     it would not slow down the executor.

Interesting. Have you got anything for it now? Anyway, what is the actual
difference between it and the fast cache facility..?

Moriyohi

Zeev Suraski

22 years ago

At 15:16 05/10/2003, Thies C. Arntzen wrote:    on a sidenote - i once
played around by just adding this

>     garbage to a linked list and only reclaim it inside emalloc.
>     that way we don't need to clean it after every op.

We plan to completely remove the garbage mechanism after beta 2.

Zeev