HaxeFoundation / haxe

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

Do not use IMap. #6394

Open hughsando opened 7 years ago

hughsando commented 7 years ago

Using the IMap interface creates significant overhead on hxcpp - and possibly all strongly typed targets. Since we do not have multiple implementations of IMap, the abstraction seems unnecessary. We should certainly remove this unexpected use of an interface from the top-level "Map" class, which appears to be a shourtcut for haxe.ds.*Map, but in fact is some kind of interface abstraction.

Simn commented 7 years ago

I don't like Map either, but IMap is not supposed to show up in the output because it should be specialized away.

hughsando commented 7 years ago

The 'Map' is abstracted over the IMap as 'this' so every time you make a member call, it is a member call on the IMap - 'this'. ie, an interface call.

Simn commented 7 years ago

Yes, but then @:multiType kicks in and specializes it:

class Main {
    static public function main() {
        var map = new Map();
        map["foo"] = "bar";
    }
}

Dump:

@:keep @:used
class Main {

    @:keep
    public static function main[Function:Void -> Void]
        [Block:Void]
            [Call:Void]
                [Field:key : String -> value : String -> Void]
                    [Cast:Map<String, String>] [New:haxe.ds.StringMap<String>] haxe.ds.StringMap<String>
                    [FInstance:key : String -> value : String -> Void]
                        haxe.ds.StringMap<String>
                        set
                [Const:String] "foo"
                [Const:String] "bar"
}

If there are any IMap leftovers in the AST, we have to fix that. But for that you'll have to tell me how to reproduce it first.

hughsando commented 7 years ago

Yes, it seems to be ok in this example - I'll dig up the example I was looking at.

hughsando commented 7 years ago

It is to do with the use of @:generic - so maybe a corner case.

@:generic
class Mapper<T>
{
   #if use_map
   var map = new Map<String,T>();
   #else
   var map = new haxe.ds.StringMap<T>();
   #end

   public function new() { }

   public function set(name:String, t:T)map.set(name,t);

   public function exists(name:String) return map.exists(name);
}

class Test
{
   public static function main()
   {
      var mapper = new Mapper<Test>();
      mapper.set("hello", null);
      trace(mapper.exists("hello"));
      trace(mapper.exists("jello"));
   }
}

Still, I would be glad to see IMap removed. Otherwise people who love interfaces will be tempted to use it.

Simn commented 7 years ago

I still don't know what to do with Map in general. As long as it's a normal abstract, we need an underlying type and that can only be an interface, I think. We could experiment with making it a @:coreType abstract.

saem commented 4 years ago

Abstract interfaces or structural types as something that are explicitly compile time only might help.

Likey needs a proposal, but having this and other "higher order" abstracts might indicate a desire for something like row polymorphism (compile time only). The type following with platform specialization like in Map<K,V> or trying to use @:generic on an abstract with a type parameter seems to all go after similar things.

hughsando commented 4 years ago

KeyValueIterator, and indeed Iterator is another construct that has terrible performance characteristics on static targets - basically requiring dynamic access for the fields, and (for hxcpp) dynamic access of the value. Anything that could be done to make this strongly typed would be good.