haxetink / tink_core

Core utilities
https://haxetink.github.io/tink_core
MIT License
117 stars 33 forks source link

Future.NEVER is wrapped when used as Future<Noise> #153

Closed nadako closed 3 years ago

nadako commented 3 years ago
import tink.core.Future;
import tink.core.Noise;

function main() {
    var f:Future<Noise> = Future.NEVER;
}

generates

function Main_main() {
    let f = tink_core_Future.noise(tink_core_Future.NEVER);
}
nadako commented 3 years ago

I think it was introduced in 71b86e62c046273f8f226c8b4e3a3177ffac5e98 and it happens because of the @:to, which I don't particularly like, but it's consistent with Promise. Also, Promise.NEVER has (and had before) the same issue.

back2dos commented 3 years ago

Actually, this wasn't really much of a problem, because map/flatMap noops out on futures with a status of NeverEver. I initially tackled this with a more prioritized implicit cast, but I'd rather avoid that, since how these work tends to change between minor Haxe versions.

There was a remaining allocation for the local function that would map / next the result to Noise, which of course is not used for NEVER. This is now avoided: https://github.com/haxetink/tink_core/commit/396705beabca0e97decb66774be6d056b0d904f1

This doesn't come 100% free at runtime as the implicit cast would, but the advantage is that Future.NEVER under the guise of a different type will still benefit from this fast path.

nadako commented 3 years ago

Could we have both the run-time fast path and implicit cast tho? :)

back2dos commented 3 years ago

Hmm, I'm not blown away by the benefits:

import tink.core.Promise;
import haxe.Timer.*;

function main() {
  measure(() ->
    for (i in 0...1000000000)
      Promise.NEVER.noise()
  );

  measure(() ->
    for (i in 0...1000000000)
      {}
  );
}

I get numbers like these:

haxe -dce full -lib tink_core -js out.js -main Test && node out.js
Test.hx:5: 1.2309999465942383s
Test.hx:10: 0.49499988555908203s
back2dos commented 3 years ago

That said, if you're willing to maintain these casts for the particular Haxe 4 versions that interest you (falling back to runtime behavior in older releases is fine by me), then feel free to do so. For testing I would suggest overwriting noise at runtime in #if js, so that you can assert it's not being called.