perf: class_ and classes -> dont use unwrap because it generates inefficient code, use unsafeCoerce

[srghma]

before purs 0.13.8 generated

var classes = (function () {
    var $14 = prop(Halogen_HTML_Core.isPropString)("className");
    var $15 = Data_String_Common.joinWith(" ");
    var $16 = Data_Functor.map(Data_Functor.functorArray)(Data_Newtype.unwrap(Halogen_HTML_Core.newtypeClassName));
    return function ($17) {
        return $14($15($16($17)));
    };
})();
var class_ = (function () {
    var $18 = prop(Halogen_HTML_Core.isPropString)("className");
    var $19 = Data_Newtype.unwrap(Halogen_HTML_Core.newtypeClassName);
    return function ($20) {
        return $18($19($20));
    };
})();

now

var classes = (function () {
    var $14 = prop(Halogen_HTML_Core.isPropString)("className");
    var $15 = Data_String_Common.joinWith(" ");
    return function ($16) {
        return $14($15($16));
    };
})();
var class_ = (function () {
    var $17 = prop(Halogen_HTML_Core.isPropString)("className");
    return function ($18) {
        return $17($18);
    };
})();

[thomashoneyman]

I think it would be better to wait for 0.14, which implements safe coercions so that we get this for free without using unsafeCoerce (we can use the ‘coerce’ function instead).

[srghma]

@thomashoneyman will it? I thought it needs something like https://github.com/purescript/purescript/issues/3890 to work

[thomashoneyman]

Hmm — I didn’t know about #3890. I would hope that if coerce is meant to be the way to do safe coercions that it would generate the same code as unsafeCoerce.

This change is probably not very risky, and it generates better code, but if possible I would rather rely on a safe operation and I’d like to check whether coerce will provide that. Halogen 6 will be released with 0.14. Even if it doesn’t generate the nicer code it at least still has some savings.

I would also hope that the newtype library uses coerce for unwrap going forward, because afaik that’s the primary thing to replace with it. In that case we wouldn’t need to change any uses of unwrap.

Have you checked for other places where we should plan to switch to coerce?

[srghma]

Have you checked for other places where we should plan to switch to coerce?

no :eyes: )

[kritzcreek]

Why don't you just pattern match the newtype?

[thomashoneyman]

Why don't you just pattern match the newtype?

Good point. Though that wouldn't help in the case of map unwrap, where we can coerce the container itself, right?

[kritzcreek]

Though that wouldn't help in the case of map unwrap, where we can coerce the container itself, right?

Yeah, that's right

[thomashoneyman]

After looking further through the plans for safe coercion and #3890 I would rather not introduce the unsafeCoerce calls.

Instead, I suggest we pattern-match on the newtype instead of calling unwrap for class_, and that we use coerce to skip the call to map unwrap (as well as anywhere else in the codebase this pattern exists) once 0.14 is ready.

I don't think the small additional overhead of using coerce as an alias for unsafeCoerce is worth switching to unsafeCoerce directly. I would rather stick with coerce and work towards that being inlined in the compiler if the overhead is an issue. That way the whole ecosystem can benefit from the optimization.

[srghma]

with coerce the generated core is

var classes = (function () {
    var $12 = prop(Halogen_HTML_Core.isPropString)("className");
    var $13 = Data_String_Common.joinWith(" ");
    var $14 = Safe_Coerce.coerce();
    return function ($15) {
        return $12($13($14($15)));
    };
})();
var class_ = (function () {
    var $16 = prop(Halogen_HTML_Core.isPropString)("className");
    var $17 = Safe_Coerce.coerce();
    return function ($18) {
        return $16($17($18));
    };
})();

 ~/projects/purescript-halogen   patch-3  purs --version                                                                         <<<
0.14.0
 ~/projects/purescript-halogen   patch-3  spago version                                                                          <<<
0.20.0

why it doesnt optimize?