HaxeFoundation / haxe

Haxe - The Cross-Platform Toolkit
https://haxe.org
6.14k stars 655 forks source link

[cpp] Generic functions not resulting in optimally typed code #4894

Open gamedevsam opened 8 years ago

gamedevsam commented 8 years ago

In HaxeFlixel, we have a class FlxDestroyUtil.hx, it has the following function:

/**
 * Checks if an object is not null and exists before calling destroy(), always returns null.
 * 
 * @param   object  An IFlxExisting object that will be destroyed if it's not null and exists.
 * @return  null
 */
@:generic public static function destroyExisting<T:IFlxExisting>(object:T):T
{
    if (object != null && object.exists)
    {
        object.destroy();
    }
    return null;
}

That function, when called from FlxLinkedList class (which implements IFlxExisting), results in the following cpp code:

::flixel::_system::FlxLinkedList FlxDestroyUtil_obj::destroyExisting_flixel_system_FlxLinkedList( ::flixel::_system::FlxLinkedList object){
    HX_STACK_FRAME("flixel.util.FlxDestroyUtil","destroyExiting_flixel_system_FlxLinkedList",0xaa3cfb51,"flixel.util.FlxDestroyUtil.destroyExisting_flixel_system_FlxLinkedList","flixel/util/FlxDestroyUtil.hx",32,0xaf22421a)
    HX_STACK_ARG(object,"object")
    HX_STACK_LINE(33)
    bool tmp = (object != null());      HX_STACK_VAR(tmp,"tmp");
    HX_STACK_LINE(33)
    bool tmp1;      HX_STACK_VAR(tmp1,"tmp1");
    HX_STACK_LINE(33)
    if ((tmp)){
        HX_STACK_LINE(33)
        tmp1 = object->__Field(HX_HCSTRING("exists","\xdc","\x1d","\xe0","\xbf"), hx::paccDynamic );
    }
    else{
        HX_STACK_LINE(33)
        tmp1 = false;
    }
    HX_STACK_LINE(33)
    if ((tmp1)){
        HX_STACK_LINE(35)
        object->__Field(HX_HCSTRING("destroy","\xfa","\x2c","\x86","\x24"), hx::paccDynamic )();
    }
    HX_STACK_LINE(37)
    return null();
}

STATIC_HX_DEFINE_DYNAMIC_FUNC1(FlxDestroyUtil_obj,destroyExiting_flixel_system_FlxLinkedList,return )

It appears Haxe generated the correct type for the parameter ::flixel::_system::FlxLinkedList object, but is still using __Field to access members of that object for some reason.

Simn commented 8 years ago

Can you reduce that to a minimal example?

delahee commented 8 years ago

The problem is well identified, It is that non inline Genericity implies Dynamic, it is a very big performance problem, but we could only solve it by creating one code instance by type parameter, that would require big work and additionnal metas I think...

anyway big +1 for this.

2016-03-03 20:26 GMT+01:00 Simon Krajewski notifications@github.com:

Can you reduce that to a minimal example?

— Reply to this email directly or view it on GitHub https://github.com/HaxeFoundation/haxe/issues/4894#issuecomment-191925245 .

David Elahee

gamedevsam commented 8 years ago

I could not reproduce the issue in a simplified project. I tried to make it as similar as to the HaxeFlixel use case as possible to trigger the behavior, but it output perfectly typed code.

Here's my failed attempt reproducing the issue: https://dl.dropboxusercontent.com/u/23286216/Haxe/GenericFunctions.zip

Here's the HaxeFlixel project (with flixel source) that reproduces the issue: https://dl.dropboxusercontent.com/u/23286216/Haxe/FlxCollisions.zip

Here is how the simplified project compares to HaxeFlixel:

HaxeFlixel (FlxLinkedList.hx):

class FlxLinkedList implements IFlxExisting

GenericFunction (LinkedList.hx):

class LinkedList implements IExisting

HaxeFlixel (FlxDestroyUtil.hx):

interface IFlxDestroyable
{
    function destroy():Void;
}

interface IFlxExisting extends IFlxDestroyable 
{
    var exists:Bool;
}

class FlxDestroyUtil
{
    /**
     * Checks if an object is not null and exists before calling destroy(), always returns null.
     * 
     * @param   object  An IFlxExisting object that will be destroyed if it's not null and exists.
     * @return  null
     */
    @:generic public static function destroyExisting<T:IFlxExisting>(object:T):T
    {
        if (object != null && object.exists)
        {
            object.destroy();
        }
        return null;
    }
}

GenericFunction (DestroyUtil.hx):

interface IDestroyable
{
    function destroy():Void;
}

interface IExisting extends IDestroyable 
{
    var exists:Bool;
}

class DestroyUtil
{
    @:generic public static function destroyExisting<T:IExisting>(object:T):T
    {
        if(object != null && object.exists) 
        {
            object.destroy();
        }
        return null;
    }
}

HaxeFlixel (FlxDestroyUtil.cpp):

::flixel::_system::FlxLinkedList FlxDestroyUtil_obj::destroyExisting_flixel_system_FlxLinkedList( ::flixel::_system::FlxLinkedList object){
    HX_STACK_FRAME("flixel.util.FlxDestroyUtil","destroyExisting_flixel_system_FlxLinkedList",0xb1abc6b8,"flixel.util.FlxDestroyUtil.destroyExisting_flixel_system_FlxLinkedList","flixel/util/FlxDestroyUtil.hx",42,0xaf22421a)
    HX_STACK_ARG(object,"object")
    HX_STACK_LINE(43)
    bool tmp = (object != null());      HX_STACK_VAR(tmp,"tmp");
    HX_STACK_LINE(43)
    bool tmp1;      HX_STACK_VAR(tmp1,"tmp1");
    HX_STACK_LINE(43)
    if ((tmp)){
        HX_STACK_LINE(43)
        tmp1 = object->__Field(HX_HCSTRING("exists","\xdc","\x1d","\xe0","\xbf"), hx::paccDynamic );
    }
    else{
        HX_STACK_LINE(43)
        tmp1 = false;
    }
    HX_STACK_LINE(43)
    if ((tmp1)){
        HX_STACK_LINE(45)
        object->__Field(HX_HCSTRING("destroy","\xfa","\x2c","\x86","\x24"), hx::paccDynamic )();
    }
    HX_STACK_LINE(47)
    return null();
}

GenericFunction (DestroyUtil.cpp):

::LinkedList DestroyUtil_obj::destroyExisting_LinkedList( ::LinkedList object){
    HX_STACK_FRAME("DestroyUtil","destroyExisting_LinkedList",0x55dda0c3,"DestroyUtil.destroyExisting_LinkedList","DestroyUtil.hx",16,0x360fda42)
    HX_STACK_ARG(object,"object")
    HX_STACK_LINE(17)
    bool tmp = (object != null());      HX_STACK_VAR(tmp,"tmp");
    HX_STACK_LINE(17)
    bool tmp1;      HX_STACK_VAR(tmp1,"tmp1");
    HX_STACK_LINE(17)
    if ((tmp)){
        HX_STACK_LINE(17)
        tmp1 = object->exists;
    }
    else{
        HX_STACK_LINE(17)
        tmp1 = false;
    }
    HX_STACK_LINE(17)
    if ((tmp1)){
        HX_STACK_LINE(19)
        object->destroy();
    }
    HX_STACK_LINE(21)
    return null();
}

PS: This has nothing to do with inline vs non-inline. The inlined code also uses __Field operations int this case.

hughsando commented 8 years ago

I wonder if it is getting confused be the variable in the interface. Your generic function will take anything that implements in the interface, but then uses generic to avoid actually using the interface, which seems ok. But if you replace the requirement of implementing the interface with the restriction that the object has an 'exists' and a 'destroy', does that help ? { var exists:Bool; function destroy():Void; } This might hint at the solution.

gamedevsam commented 8 years ago

That did not change the output, same thing. What boggles my mind is that the function parameter is correctly typed, but still uses __Field for accessing its public members, so weird.