inline recursion

[ncannasse]

typedef List = { v : Int, next : List };

class Test {

    static inline function cmp(a:List,b:List) {
        return a.v - b.v;
    }

    static function main() {
        var l : List = { v : 0, next : { v : -1, next : null }};
        haxe.ds.ListSort.sortSingleLinked(l,function(a,b) return a.v - b.v);
        haxe.ds.ListSort.sortSingleLinked(l,cmp);
        trace(l);
    }
}

The first ListSort will correctly inline the local function, but the cmp function will not be inlined, while it could/should.

PS: on JS, both are not inlined while they should

[Atry]

How about haxe.ds.ListSort.sortSingleLinked(l,cmp.bind());?

[Simn]

Wow, these methods in ListSort are massive. I'm not surprised the JS target refuses to inline them.

[ncannasse]

The idea is to inline the comparison function, this produces very efficient sorting since there is no function call at all. It's an important feature performance wise when working on linked lists. I would suggest we unit test it to prevent regressions.

[Simn]

I always roll my eyes when you talk about performance related to List. But yes we should add inlining tests.

[ncannasse]

@Simn it's not about List, it's about anything that you can declare that has a "next" member variable

[Simn]

What exactly is the scope here for inlining functions if they are referenced like that? Would we also inline their expression if you did var f = cmp;, or is this supposed to be special to arguments to inline functions (which seems strange)?

[ncannasse]

inline functions which takes a function literal as argument should inline its content.

[Simn]

I have implemented this but I didn't think about it thoroughly enough and I also find it too newfeaturesque for 3.2. Feel free to activate it if you think it's safe, otherwise I'll get back to it after 3.2.

[ncannasse]

@Simn we have that already in reduce_loop, don't we ?

[ncannasse]

I was triggered before we moved inline to local vars maybe ? Maybe we shouldn't do that for TFunction, and that would take us back to original optimized behavior

[ncannasse]

PS : I also want that we add an unit test for that, it's a huge regression speed-wise

[Simn]

You keep mentioning TFunction but here we only have a TField which is not safe to inline in the general case. Though I think it can actually be inlined if it's a Method (not MethDynamic).

I didn't know that this was working at some point and that there was this code in replace_loop. In that case I agree we should look into it for 3.2.

[ncannasse]

Ah, right, only the first was supported before, but it seems on JS this was disabling inlining, so I had to rely on the second behavior. If possible I would like to get both to work well for 3.2

[Simn]

I didn't have this on my radar because it was assigned to you and now it's too late to make any changes here. Moving to 3.3 again.

[Simn]

So why would it be crazy to simply use the function expression (with replaced locals) if we take the "closure" of a static inline method? This goes very well with the general concept of inlining and solves this particular problem as a subset.

[ncannasse]

@Simn I'm not sure I'm understanding what you're proposing here :)

[Simn]

typedef List = { v : Int, next : List };
class Main {
    static inline function cmp(a:List, b:List) {
        return a.v - b.v;
    }

    static function main() {
        var f = cmp;
    }
}

->

    static function main() {
        var f = function(a:List, b:List) {
            return a.v - b.v;
        }
    }

[ncannasse]

How does that help ? it will be actually slower unless "f" calls are inlined, and harder to optimize given that now f is a variable that can be written.

[Simn]

It helps because it turns:

haxe.ds.ListSort.sortSingleLinked(l,cmp);

to

haxe.ds.ListSort.sortSingleLinked(l,function(a,b) return a.v - b.v);

which happens to be the case that's already optimized.

[ncannasse]

Oh, right, got it. So, if we want to substitute the function expr when calling type_inline, it's fine. But we shouldn't do that if inlining is canceled or if no inlining is done.

[Simn]

I don't understand what problems always inlining them like that would cause. In my opinion taking the "closure" of a static inline function should do exactly that.

[ncannasse]

If sortSingleLinked is not inline, it means we will allocate a closure each time we call it, instead of simply passing the function pointer. The same if for some reason sorteSingleLinked modify its cmpFunc parameter which will prevent from inlining the call to it

[Simn]

We could solve this as part of the data flow analysis. It's an easy change but we have to drag the typer context into the analyzer because type_inline requires it.