[DICUSS]Cleanup resource handling APIs

Hey Sara:

furthermore, I'd like to discuss remove the handle in zend_resource struct..

it may breaks some usage (use resource as long/double/string

I've seen uses in the wild where file handles are used as array keys (yes, I know, ugly and wrong).

If we were to remove rsrc handle, we should formally deprecate it for at least one minor revision first.

I'm all for killing it, and I fully support improving the internal API, but the gentler we are with BC breaks, the better.
I agree, as I said, removing it is the step 2 actions

we may discuss it later..

thanks

-Sara

--
Xinchen Hui
@Laruence
http://www.laruence.com/

furthermore, I'd like to discuss remove the handle in zend_resource struct..

it may breaks some usage (use resource as long/double/string

I've seen uses in the wild where file handles are used as array keys (yes, I know, ugly and wrong).

Yes, I definitely use that a lot and cannot see why this should be wrong.

If we were to remove rsrc handle, we should formally deprecate it for at least one minor revision first.

I'm all for killing it, and I fully support improving the internal API, but the gentler we are with BC breaks, the better.

Hey:

On Tue, Feb 3, 2015 at 12:37 AM, François Laupretre
francois@tekwire.net wrote:

De : Xinchen Hui [mailto:laruence@php.net]
furthermore, I'd like to discuss remove the handle in zend_resource struct..

it may breaks some usage (use resource as long/double/string)

case IS_RESOURCE: {
char buf[sizeof("Resource id #") + MAX_LENGTH_OF_LONG];
int len;

        len = snprintf(buf, sizeof(buf), "Resource id #"

ZEND_LONG_FMT, (zend_long)Z_RES_HANDLE_P(op));
return zend_string_init(buf, len, 0);
}

I don't understand how you can delete the resource if you remove the handle from the zend_resource struct. Or would you store the index elsewhere ?

if you use HashTable , yes, it's hard to get rid of it, but we can
hidden it from user land..

and actually, we only use integer index of regular_list for zend_resource..

if we decide to remove handle, then it maybe easy to re-implement it
as a plain c array like zend_resource[];

and when doing realloce, we allocate new segment, and link them togther. etc

thanks

Regards

François

--
Xinchen Hui
@Laruence
http://www.laruence.com/

Hey:

Hey:

De : Xinchen Hui [mailto:laruence@php.net]
we used to use lval of zval as a handle to access resource type..

 but now, we introduced a new type IS_RESOURCE, which make the

handle(id) sort of redundant .

Wrong. The IS_RESOURCE type has nothing to do with PHP 7. Only zend_resource is new. And handle is not redundant.
of course it's a typo. I meant zend_resource

 further more, the common usage when handling resource is like:

  if (zend_parse_parameters(ZEND_NUM_ARGS(), "rl", &result,

&offset) == FAILURE) {
return;
}
ZEND_FETCH_RESOURCE(mysql_result, MYSQL_RES *, result, -1, "MySQL
result", le_result);

as you can see, we use "r" to receive a IS_RESOURCE type, that

means, check the type in ZEND_FETCH_RESOURCE is overhead..

There's no overhead here. Zend_parse_parameters checks that received arg is IS_RESOURCE. Fetch then checks that received resource is one of the accepted resource types. Sorry to say that, but are you sure you understand the difference between zval types and resource types ?
..... do you really read the FETCH_RESOURCE?

ZEND_API void *zend_fetch_resource(zval *passed_id, int default_id,
const char *resource_type_name, int *found_resource_type, int
num_resource_types, ...)
{
int actual_resource_type;
// void *resource;
va_list resource_types;
int i;
zend_resource *res;
const char *space;
const char *class_name;

if (default_id==-1) { /* use id */
    if (!passed_id) {
        if (resource_type_name) {
            class_name = get_active_class_name(&space);
            zend_error(E_WARNING, "%s%s%s(): no %s resource

supplied", class_name, space, get_active_function_name(),
resource_type_name);
}
return NULL;
} else if (Z_TYPE_P(passed_id) != IS_RESOURCE) { // < === what are this?
if (resource_type_name) {
class_name = get_active_class_name(&space);
zend_error(E_WARNING, "%s%s%s(): supplied argument is
not a valid %s resource", class_name, space,
get_active_function_name(), resource_type_name);
}
return NULL;
}

ZEND_API void *zend_fetch_resource(zval *passed_id, int default_id,
const char *resource_type_name, int *found_resource_type, int
num_resource_types, ...)

we use va_args to passing resource type, that means, the rescue
type arguments can not be passed by register but by stack.. which is a
little low effiicient .

What do you mean with 'rescue' type ?
expected resource_type

Fetch is supposed to check for a variable number of possible resource types. It could probably be restricted to 2 possible types as, generally, it is the maximum (one for non-persistent, one for persistent). But I am not sure the overhead of passing arg on the stack justifies a change. Remember that id is searched in an array, which takes probably much more time that pushing/popping one or two arguments.

so, I'd like propose a zend_resource handling API cleanup..

1. DROP ZEND_REGISTER_RESOURCE/FETCH_RESOURCE.

2. add :

   ZEND_API void *zend_fetch_resource(zend_resource *res, const
   char *resource_type_name, int resource_type);
   ZEND_API void *zend_fetch_resource2(zend_resource *res, const char
   *resource_type_name, int *found_type, int resource_type, int
   resource_type2);
   ZEND_API void *zend_fetch_resource_ex(zval *res, const char
   *resource_type_name, int resource_type);
   ZEND_API void *zend_fetch_resource2_ex(zval *res, const char
   *resource_type_name, int *found_type, int resource_type, int
   resource_type2);

If you drop ZEND_REGISTER_RESOURCE, how do you register new resources ? Or do you mean you don't register them any more ? But registering them is mandatory if we want them to be freed when request ends.

furthermore, I'd like to discuss remove the handle in zend_resource struct..

it may breaks some usage (use resource as long/double/string)

case IS_RESOURCE: {
char buf[sizeof("Resource id #") + MAX_LENGTH_OF_LONG];
int len;

       len = snprintf(buf, sizeof(buf), "Resource id #"

ZEND_LONG_FMT, (zend_long)Z_RES_HANDLE_P(op));
return zend_string_init(buf, len, 0);
}

OK. You want to remove resource registration. But resources don't work this way (see http://devzone.zend.com/446/extension-writing-part-iii-resources/). If you remove the handle, you remove the whole zend_list API.

The zend_resource struct is not a structure you may fill with random data. Using the handle to store long/double/string is not a legitimate usage.
....I think you don't understand what I am talking about. sorry
I think I'd better to explain a bit more.

we used to use handle to lookup a resource in EG(regular_list).. and
store the "int handle" into lval of zval.

but now, we use zend_resource * , which can be directly accessed via
Z_RES_P(zval).

that's why I think the "handle" is a little redundant ..

is that clear?

thanks

thanks

Regards

François

--
Xinchen Hui
@Laruence
http://www.laruence.com/

--
Xinchen Hui
@Laruence
http://www.laruence.com/

Hi,

Hey:

On Mon, Feb 2, 2015 at 2:51 PM, François Laupretre francois@tekwire.net
wrote:

De : Xinchen Hui [mailto:laruence@php.net]
we used to use lval of zval as a handle to access resource type..

but now, we introduced a new type IS_RESOURCE, which make the
handle(id) sort of redundant .

Wrong. The IS_RESOURCE type has nothing to do with PHP 7. Only
zend_resource is new. And handle is not redundant.
of course it's a typo. I meant zend_resource

further more, the common usage when handling resource is like:

if (zend_parse_parameters(ZEND_NUM_ARGS(), "rl", &result, &offset) ==
FAILURE) {
return; }
ZEND_FETCH_RESOURCE(mysql_result, MYSQL_RES *, result, -1, "MySQL
result", le_result);

as you can see, we use "r" to receive a IS_RESOURCE type, that means,
check the type in ZEND_FETCH_RESOURCE is overhead..

There's no overhead here. Zend_parse_parameters checks that received
arg is IS_RESOURCE. Fetch then checks that received resource is one of
the accepted resource types. Sorry to say that, but are you sure you
understand the difference between zval types and resource types ?
..... do you really read the FETCH_RESOURCE?

ZEND_API void *zend_fetch_resource(zval *passed_id, int default_id,
const char *resource_type_name, int *found_resource_type, int
num_resource_types, ...) {
int actual_resource_type; // void *resource;
va_list resource_types; int i; zend_resource *res; const char *space; const
char *class_name;

if (default_id==-1) { /* use id */ if (!passed_id) { if
(resource_type_name) {
class_name = get_active_class_name(&space); zend_error(E_WARNING,
"%s%s%s(): no %s resource
supplied", class_name, space, get_active_function_name(),
resource_type_name); }
return NULL; } else if (Z_TYPE_P(passed_id) != IS_RESOURCE) { // < === what
are this? if (resource_type_name) { class_name =
get_active_class_name(&space); zend_error(E_WARNING, "%s%s%s(): supplied
argument is not a valid %s resource", class_name, space,
get_active_function_name(), resource_type_name); }
return NULL; }

ZEND_API void *zend_fetch_resource(zval *passed_id, int default_id,
const char *resource_type_name, int *found_resource_type, int
num_resource_types, ...)

we use va_args to passing resource type, that means, the rescue type
arguments can not be passed by register but by stack.. which is a
little low effiicient .

What do you mean with 'rescue' type ?

expected resource_type

Fetch is supposed to check for a variable number of possible resource
types. It could probably be restricted to 2 possible types as,
generally, it is the maximum (one for non-persistent, one for
persistent). But I am not sure the overhead of passing arg on the stack
justifies a change. Remember that id is searched in an array, which
takes probably much more time that pushing/popping one or two
arguments.

so, I'd like propose a zend_resource handling API cleanup..

1. DROP ZEND_REGISTER_RESOURCE/FETCH_RESOURCE.

2. add :

ZEND_API void *zend_fetch_resource(zend_resource *res, const
char *resource_type_name, int resource_type); ZEND_API void
*zend_fetch_resource2(zend_resource *res, const char
*resource_type_name, int *found_type, int resource_type, int
resource_type2); ZEND_API void *zend_fetch_resource_ex(zval *res, const
char *resource_type_name, int resource_type);
ZEND_API void *zend_fetch_resource2_ex(zval *res, const char
*resource_type_name, int *found_type, int resource_type, int
resource_type2);

If you drop ZEND_REGISTER_RESOURCE, how do you register new resources ?
Or do you mean you don't register them any more ? But registering them
is mandatory if we want them to be freed when request ends.

furthermore, I'd like to discuss remove the handle in zend_resource
struct..

it may breaks some usage (use resource as long/double/string)

case IS_RESOURCE: { char buf[sizeof("Resource id #") +
MAX_LENGTH_OF_LONG];
int len;

len = snprintf(buf, sizeof(buf), "Resource id #" ZEND_LONG_FMT,
(zend_long)Z_RES_HANDLE_P(op));
return zend_string_init(buf, len, 0); }

OK. You want to remove resource registration. But resources don't work
this way (see
http://devzone.zend.com/446/extension-writing-part-iii-resources/). If
you remove the handle, you remove the whole zend_list API.

The zend_resource struct is not a structure you may fill with random
data. Using the handle to store long/double/string is not a legitimate
usage.
....I think you don't understand what I am talking about. sorry

I was writing about it last year as well
http://marc.info/?l=php-internals&m=142006455308305&w=2 , slightly
different idea. Annd there is also a short piece of code to illustrate:
https://gist.github.com/weltling/9367db5e242ef7e60042 ...

The integer resource looks like is needed at some places, so my suggestion
was to split zend_fetch_resource function like illustrated in the patch.
At 99% of places that integer link is just passed as -1, so then having it
be present always in the signature is just a waste of "resources" :)

Regards

Anatol