[Xapian-discuss] Another PHP 5 wrapper...

[Olly Betts]

On Tue, Apr 18, 2006 at 07:33:18PM +0200, Daniel M?nard wrote:
> >I've used Paul's ideas for coercing parameters and return values, except
> >that I'm generating wrapper methods so I can skip doing that when I know
> >it won't be needed (currently the check errs on the side of coercing
> >when it sometimes isn't needed which doesn't affect correctness, but
> >just adds a small overhead).

I've done a fair bit more work since the patch and output I uploaded.
Functions and static methods are now wrapped, and I've eliminated
calls to array_walk_recursive with swig_coerce_param in cases where
there's no parameter which is a wrapped class in any overloaded version
(suprisingly few Xapian methods take other Xapian objects).

Here's the latest version:

http://www.oligarchy.co.uk/xapian/patches/swig-php-gen-oo-wrappers5.patch
http://www.oligarchy.co.uk/xapian/patches/xapian5.phps

> It is probably a good thing to generate wrapper methods and not using 
> the "__call" magic function of php, because it seems to add a 
> substantial overhead.
> [...]
> On my machine, results are as follows :
> - using native calls : 8,88 seconds
> - using my own wrapper : 13,88 sec
> - using Paul's wrapper : 64,12 sec
> - using the wrapper generated by SWIG : 37,54 sec.
>[...]
> SWIG's wrapper don't use __call, but it seems that using 
> func_get_args/array_walk_recursive/array_unshift/call_user_func_array 
> for each call still takes time.

It'd be interesting to see how the latest version does then - if it's
much better, then func_get_args and call_user_func_array aren't so
much to blame.  If it's much the same, they are.

> I saw in your patch that we have access to all the parameters of the 
> function we wrap. Perhaps it would be possible to write all the 
> parameters of the generated PHP method and then do a direct (real) call 
> to the flat Xapian function, coercizing (?) on the fly all the 
> parameters for which it is necessary.

The word you're after is "coercing"!

We can certainly do this for methods which aren't overloaded (and don't
have default parameters).  I was wondering about looking at that next
already actually.

Overloaded methods which all have the same number of parameters in each
version are pretty easy too - we just need to check and potentially
coerce parameter positions which could be objects.

We might be able to handle a few more cases, but the arbitrary case
would be tricky to handle I think, and the amount of checking needed
will add overhead, so it might be as quick to just do as we currently
do.  It depends how much of the overhead is in
func_get_args+call_user_func_array vs array_walk_recursive vs
swig_coerce_param.

> I am pretty sure that it would be more complicated than that (handling 
> parameters default values, for example), but there would be some benefits :

I don't think we know the default values in SWIG (just which parameters
have them).

> >(in a response to Sam about swig_coerce_param) It's also nice that we can 
> >automatically do it recursively in PHP - this means that the 
> >typemap for constructing a Query from a PHP array of Query 
> >objects just works without having to rewrite it.
>
> My pseudo solution above won't handle that, except if we add a special 
> case for that (if arg is an array, walk the array to detect things to 
> coerce and create a new array). But do we really need it ? only 
> new_query uses this (?), and it seems to me that it is already very 
> flexible...

We can fix it in the custom code which implements new_Query(OP, ARRAY)
anyhow.

> Perhaps that it should be a swig option (on : camelize method names, off 
> : use method names "as-is")

It will be (SWIG already allows you to say "rename methods to upper
camel case" and "rename methods to underscore separated words" so
I just added a new option there).

> >SWIG's Python backend allows you to generate pydoc
> >strings (either automatically, or specified explicitly, or a mix of
> >the two).  There's no good reason why the PHP backend couldn't too.
>
> It would be fantastic to have the same thing for PHP, but does it mean 
> that you have to "duplicate" the doc in xapian.i or something like that?

If you use the "automatic" option it doesn't, but you just get the
method prototype in the documentation comment.

It'd be neat if SWIG could assimilate the Doxygen comments from the C++
headers (and then we'd only need to add and maintain overridden versions
for methods where those aren't suitable).  I don't think it currently
can though.

Another way to approach it might be to postprocess Doxygen's XML output
(which would allow mechanical changes to match PHP syntax).

> I think that we can all live with a wrapper without the "Countable" 
> interface...

We can always add it later when PHP 5.0 support is no longer an issue.

> Using it and supporting both php 5.0 and php 5.1 would require to add 
> something like this in the wrapper :
>    if (! interface_exists('Countable'))
>    {
>        interface Countable{};
>    }
> 
> But PHP function 'interface_exists' was only added in php 5.0.2, so it 
> is not a perfect solution.

What about:

    if (!function_exists('interface_exists') ||
	!interface_exists('Countable'))
    {
	interface Countable{};
    }

I have PHP 5.0.5 so I can't check if that works on 5.0.0 or 5.0.1
easily...

> Some additional remarks about the current wrapper :
> 
> I think we should avoid, if we can, having global functions or constants 
> in the generated wrapper (avoiding "global namespace pollution") :
> - I am not sure that the "$XAPIAN_LOADED__" stuff at the top of the file 
> is really necessary (php has function like include_once or 
> require_once), but perhaps it is automatically generated by SWIG ?

Yeah, SWIG always adds that.  But it can be changed if there's a better way.
I think that dates back to Sam's spell as maintainer so he may be able to
say if there's a good reason for doing it like that.

> - in Paul's wrapper, functions like swig_coerce_param and 
> swig_wrap_return were members of the base class. I think it is a better 
> solution than putting them global.

Then I can't call them from function wrappers though...

> The PHP function "is_a" (used in coerce_param) is deprecated in PHP5 ans 
> is replace by "instanceof".
>    if ($value instanceof XapianSwigBase) $value=$value->swig_ptr;

OK, changed.

> Please note that, except that, your generated wrapper doesn't have any 
> warning, even in E_STRICT mode.
> 
> You really did a great job !

Cool - it's almost like I know what I'm doing...

Cheers,
    Olly