Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:18192
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Message-ID: <65.2E.33075.C74E2034@pb1.pair.com>
To: internals@lists.php.net
Date: Wed, 17 Aug 2005 09:14:50 +0200
Organization: IWORKS
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.2) Gecko/20040803 [ http://iworks.at ]
MIME-Version: 1.0
References: <61.69.24081.E876BF24@pb1.pair.com>	<BF.79.33075.DBAACF24@pb1.pair.com> <1997493019.20050816231952@marcus-boerger.de>
In-Reply-To: <1997493019.20050816231952@marcus-boerger.de>
Content-Type: multipart/signed; micalg=pgp-sha1;
	protocol="application/pgp-signature";
	boundary="------------enigA0AE25CBEBC43BC48E6EC8B9"
Subject: Re: [PHP-DEV] Re: [PATCH] internal class' static properties and constants
From: mike@php.net (Michael Wallner)

--------------enigA0AE25CBEBC43BC48E6EC8B9
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Hi Marcus Boerger, you wrote:
> Hello Michael,
>
> a few questions/comments so that i can understand better:
>
> 1) Zend/zend_API.c: <NEW>
> +ZEND_API int zend_declare_class_constant_stringl(zend_class_entry *ce, char *name, size_t name_length, char *value, size_t value_length TSRMLS_DC)
> +{
> +       zval *constant = new_zval(ce->type & ZEND_INTERNAL_CLASS);
> +       if (ce->type & ZEND_INTERNAL_CLASS) {
> +               ZVAL_STRINGL(constant, zend_strndup(value, value_length), value_length, 0);
> +       } else {
> +               ZVAL_STRINGL(constant, value, value_length, 1);
> +       }
> +       return zend_declare_class_constant(ce, name, name_length, constant TSRMLS_CC);
> +}
> +
>
> If the internal part uses string duplication so shall use the user space
> code.

Yes, the internal part uses zend_strndup() while the userspace part
- I guess - estrndup().


> 2) Zend/zend_API.c: <NEW>
> +ZEND_API int zend_update_static_property(zend_class_entry *scope, char *name, size_t name_len, zval *value TSRMLS_DC)
> +{
> +       int retval;
> +       zval **property = NULL;
> +       zend_class_entry *old_scope = EG(scope);
> +
> +       EG(scope) = scope;
> +
> +       if (!(property = zend_std_get_static_property(scope, name, name_len, 0 TSRMLS_CC))) {
> +               retval = FAILURE;
> in the caose you utilize this value is a temp thing thus you need to free it
> in all cases but those you directly use it. A failure is definitively a
> thing where you do't use it so it must be fr here.
> +       } else if (*property == value) {
> +               retval = SUCCESS;
> Here you compare the address of a zval, was that intended?
> In case you meant it, then i think it cannot be correct because that should
> never happen, or is that the case you actually want to prevent?
> In case you meant to prohibit from setting the same value again you need
> actual zval comparison which won't work easily for strings.

This is from zend_reflection_api ReflectionProperty::setValue().
I assume it is meant to prevent useless operations,
because when else should the zvals point to the same address?

> +       } else {
> +               if (PZVAL_IS_REF(*property)) {
> +                       zval_dtor(*property);
> +                       (*property)->type = value->type;
> +                       (*property)->value = value->value;
> +
> +                       if (value->refcount) {
> +                               zval_copy_ctor(*property);
> +                       }
> +               } else {
> +                       **property = *value;
> +                       zval_copy_ctor(*property);
> +               }
> +               retval = SUCCESS;
> +       }
> +
> +       if (!value->refcount) {
> +               zval_dtor(value);
> +               FREE_ZVAL(value);
> +       }
> This only works because your zvals are initialized with refcount=0 which
> cannot be right. In the end it should always read here
> zval_ptr_dtor(&value); And tmp_zval() should initialize with refcount=1.

I actually thought that there should be an "else { zval_ptr_dtor(&value) }"?
Every place I looked at where "temporary zvals" are used, initialize them
with refcount=0.

> +
> +       EG(scope) = old_scope;
> +
> +       return retval;
> +}
>
> Your static properties get duplicated and const updated from
>
> 3) Zend/zend.c: static void class_to_unicode(zend_class_entry **ce)
> +       zend_u_hash_init_ex(&new_ce->default_static_properties, (*ce)->default_static_properties.nNumOfElements, NULL, (*ce)->default_static_properties.pDestructor, 1, 1, 0);
> +       zend_hash_copy(&new_ce->default_static_properties, &(*ce)->default_static_properties, (copy_ctor_func_t) zval_ptr_to_unicode, &tmp_zval, sizeof(zval));
> +       zend_u_hash_init_ex(&new_ce->static_properties, (*ce)->static_properties.nNumOfElements, NULL, (*ce)->default_static_properties.pDestructor, 1, 1, 0);
> Why is the above not using
> "(*ce)->default_static_properties.nNumOfElements", since that is the number
> of entries the static property table will have exactly after the copy
> operation on the next line so why not initialize the number to this?

Yeah, this is a matter of optimization which I was not thinking about
in the first place, sorry.

> +       zend_hash_copy(&new_ce->static_properties, &(*ce)->static_properties, (copy_ctor_func_t) zval_ptr_to_unicode, &tmp_zval, sizeof(zval));
>
> 4) Zend/zend_API.c: ZEND_API void zend_update_class_constants(zend_class_entry *class_type TSRMLS_DC)
>                 zend_hash_apply_with_argument(&class_type->default_properties, (apply_func_arg_t) zval_update_constant, (void *) 1 TSRMLS_CC);
> -               zend_hash_apply_with_argument(class_type->static_members, (apply_func_arg_t) zval_update_constant, (void *) 1 TSRMLS_CC);
> +               zend_hash_apply_with_argument(&class_type->default_static_properties, (apply_func_arg_t) zval_update_constant, (void *) 1 TSRMLS_CC);
> Before your patch we have the situation that consts can only be decalred in
> user space. Thus it is ensured that their data is emalloc'd and only ever
> needs to be const-updated once. Now that you have this as well as your
> default static properties it (zend_update_class_constants()) must be called
> once for every request time. Having said that i think we actuall need two
> tables. cor consts as well. I think default_consts and
> default_static_properies never need to be const updated because otherwise
> you wouldn't react on define changes if a defined const is being referred
> to. With this in mind we also face another problem, the problem that
> const-update assumes that the memory was emalloc'd.So what we need to do is
> ensuring that on the first use in a request the two tables default_const and
> default_static_properties get copied to their working tables consts and
> static_properties. After that copy we can update the consts. Now we also
> ensured that for internal classes the values in the defualt_const table and
> default_static_properties table can become malloc'd.

Well, I didn't really get the idea of this const updating yet, so apologize
if I'm totally wrong here.  Ok, so it is a problem that the const updating
routine (whatever it does) assumes that memory was emalloc()'d but why do
constants_table and default_static_properties need to be updated for
every request? They're not supposed to change their values after
declaration, are they?


> 5) Zend/zend_compile.c: ZEND_API void zend_initialize_class_data(zend_class_entry *ce, zend_bool nullify_handlers TSRMLS_DC)
> +       zend_u_hash_init_ex(&ce->default_static_properties, 0, NULL, zval_ptr_dtor_func, persistent_hashes, UG(unicode), 0);
> +       zend_u_hash_init_ex(&ce->static_properties, 0, NULL, ZVAL_PTR_DTOR, persistent_hashes, UG(unicode), 0);
> After reading 4) it should be clear that the values in
> default_static_properties must be malloc'c zval's. With this in mind we of
> course need to specify a corresponding free method insetad of the currently
> used one that assumes the memory was emalloc'd.

Hm...? default_static_properties' destructor is zval_internal_ptr_dtor, so?


> 6) Renamic static_members to static_properties is a very good idea if you
> ask me but it requires that we have this patch ready and set before 5.1 is
> out.

Well, I'm the one who most strongly hopes that, as my HttpResponse class
is just a bad hack until so-called v6.  As you probably know, I'm a first
time extension writer and still learning a lot and my knowledge of the
internals is still quite low.

Thanks,
--
Michael - < mike(@)php.net >

--------------enigA0AE25CBEBC43BC48E6EC8B9
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.0 (Cygwin)

iD8DBQFDAuPt2pTtEijQyW0RAgTNAJ9anCQszEheJ1gEap20eyLUWuHaJgCeM4mu
Zq3jWKWqOhhX1kLoDqZhoSg=
=r9lN
-----END PGP SIGNATURE-----

--------------enigA0AE25CBEBC43BC48E6EC8B9--