HaxeFoundation / haxe

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

bound method "propagation" #9268

Open nadako opened 4 years ago

nadako commented 4 years ago

Suppose we have this code:

class Future {
    function new() {}
    function trigger() {
        trace("triggered");
    }
    public static inline function async(executor:(cb:()->Void)->Void):Future {
        var future = new Future();
        executor(future.trigger);
        return future;
    }
}

class Main {
    static function main() {
        var f = Future.async(cb -> doAsyncThings(() -> cb()));
        trace(f);
    }

    static function doAsyncThings(f:()->Void) f();
}

this will currently generate:

Main.main = function() {
    var future = new Future();
    var cb = $bind(future,future.trigger);
    Main.doAsyncThings(function() {
        cb();
    });
    console.log("src/Main.hx:16:",future);
};

which is technically correct, however it's not the best performane-wise, because we don't really need the $bind here. Ideally, we would tempvar the object and "propagate" method call, i.e.:

Main.main = function() {
    var future = new Future();
    Main.doAsyncThings(function() {
        future.trigger();
    });
    console.log("src/Main.hx:16:",future);
};

(not tempvaring future here as it's already a local)

Simn commented 4 years ago

This seems pretty narrow... one problem (among possibly others) is that this might cause locals to become captured, like future in your case.

Simn commented 4 years ago

I've made a demo commit to a branch to show that this is theoretically quite easy to do in the dataflow optimization. It doesn't work for your actual example due to the local capturing situation. If that can be figured out, maybe this optimization is worth it.

Feel free to check a diff to see if this actually does anything for you. In the unit tests I'm seeing a couple instances where this kicks in.

nadako commented 4 years ago

Well that doesn't change anything in my current codebase, but that's mostly because we're just starting to use closures in general and building blocks like the Future as well. I made this issue because I've noticed something that could be generated better when I was introducing Future in the codebase and checked the generated code. :) I'm curious if that has more effect for the haxetink people :)

nadako commented 4 years ago

I'll check again after migrating our codebase to tink_core ^^

nadako commented 4 years ago

I didn't quite understood the part about "local capturing situation" though, I guess that's something we should experiment with while diffing outputs for real-world code?

Simn commented 4 years ago

It rejects the transformation with the when not (is_special_var v) guard. This is to avoid pushing non-captured locals into closures and thus turning them into captured ones, which isn't working because the filter which handles capturing has already run at this point.