IO#syncStep + IO#unsafeRun = IO#unsafeRunHere?

[armanbilge]

Drafting to get feedback on this. The basic idea is to create a convenience method that starts with IO#syncStep before falling back to the usual IO#unsafeRun methods.

The motivation for syncStep itself is already covered in #1862, the goal here is just to make it easy to use while also running the IO to completion.

Name is subject to much bikeshed of course 😁 for now, I put run "here" to mean its not shifting/yielding immediately, so in some sense it's running "here".

[armanbilge]

Reminded myself of this relevant excerpt from the comparative benchmark:

It's also worth noting that this optimization is not entirely without cost. Hogging the calling thread in this fashion and preventing it from sleeping can result in poor thread affinity in applications which call unsafeRun at many points (i.e. which aren't using IOApp, or which are using it in conjunction with legacy code). This is why Cats Effect doesn't bother optimizing for the microbenchmark case. When you call unsafeRunSync() on an IO, it immediately and proactively shifts to the runtime thread pool. This shift has a one-time performance cost when your application starts, or when a benchmark runs.

Obviously the semantics for JVM and JS are different around this, and if this goes forward maybe it should be a JS-only method.

[djspiewak]

So… I'm skeptical, but then I don't use React. :-P Intuitively, it almost feels like this belongs on SyncIO. Like:

// bikeshed name
def unsafeRunSyncAndContinue[B](
    cb: Either[Throwable, B] => Unit)(
    implicit runtime: unsafe.IORuntime, ev: A <:< Either[IO[B], B])
    : Unit =
  ev(unsafeRunSync).fold(_.unsafeRunAsync(cb), a => cb(Right(a)))

The syncStep bit feels like the part that we want to make as explicit as possible, since you're directly opting into a very different runtime semantic for IO, and ideally we wouldn't be spreading that around to too many places. From a SyncIO standpoint though, this is basically just a nice convenience method. Could also encode it separately:

def continueAsync[B](
    cb: Either[Throwable, B] => Unit)(
    implicit runtime: unsafe.IORuntime,
    ev: A <:< Either[IO[B], B])
    : SyncIO[Unit] = {

  val executed = flatMap { a =>
    SyncIO {
      ev(a).fold(
        _.unsafeRunAsync(cb),
        b => cb(Right(b)))
    }
  }

  executed.handleErrorWith(e => SyncIO(cb(Left(e))))
}

This feels even nicer. Then to get the unsafeRunHere semantic, you would call the following:

val ioa: IO[A] = ...
ioa.syncStep.continueAsync(e => ...).unsafeRunSync()

[armanbilge]

I mean, at least my thinking here was to make this easy to do, and IMHO your proposals seem kind of confusing /shrug.

since you're directly opting into a very different runtime semantic for IO

That's fair. Except, the event loop is a very different runtime than any EC on JVM. So, it's not really fair at all :P

[vasilmkd]

I think we have to accept that since 3.3.0, we're serious about Scala.js as a separate target platform for CE (it's not the case that it was just convenient to support it due to the build plugins being good), which has its own rules, quirks and different execution semantics.

With issues like #2530 popping up, I'm starting to think that maybe JS should be executed differently. And it's not going to be the only issue like this. The more we develop JS support, the more people will use it.

Splitting the runtime will be beneficial to us too, in the long run, each with its own focus, tradeoffs and optimizations.

[armanbilge]

By-and-far, the current execution model in CE lends itself very well to the JS runtime.

In fact, I'm fairly sure that all the problems on JS boil down to sometimes needing manual control of ceding/yielding. Or more specifically, being able to guarantee there isn't a cede/yield at certain points. #1862 and #2530 are specifically about avoiding that initial yield and the really weird stuff I had to do in typelevel/fs2#2663 (comment) is because of the risk of auto-cedes while registering listeners.

So if we could find a safe way to solve this, that would be marvelous. Just as we have IO.cede, maybe we need "IO.doNotCede".

[djspiewak]

That's fair. Except, the event loop is a very different runtime than any EC on JVM. So, it's not really fair at all :P

I think what I'm getting at is just trying to be really up-front about where "magic" is happening and we're breaking the rules that normally hold with respect to IO. The unsafe functions are one of those places, and so is syncStep. Having a function like continueAsync is a bit less magical, and thus separating it from syncStep and unsafeRunSync semantically makes it easier to digest, though perhaps that's just my perspective.

Auto-yielding in general can be a bit of a problem in a few places. It honestly makes me wonder if we need something more general here, though anything we do is going to quickly run up against the fact that we can't extend the typeclasses without default implementations.

[armanbilge]

we're breaking the rules that normally hold with respect to IO

Just to be clear, what rule(s) is this specifically? That unsafely running an IO always starts by shifting? Or about auto-ceding?

[djspiewak]

That unsafely running an IO always starts by shifting? Or about auto-ceding?

Both. There's also a performance cost because the syncStep interpreter is slower than IOFiber's runLoop.

[armanbilge]

Thanks. It sounds like to me that if we could just run the IOFiber without shifting, that would solve the auto-ceding issue and the performance cost. So really the question is, is the shifting rule a good one for JS? What would happen if we took it away? I can't immediately think of how someone may have come to depend on this behavior, but I don't think it's impossible.

[djspiewak]

It definitely matters for TestControl (or rather, tests written with it). That's maybe workable though but it starts to get ugly. It's also really hard for me to say whether or not someone has written code which depends on this semantic on JS. We don't have unsafeRunSync, obviously, which means that the code would take the form of something assuming that unsafeRunAndForget runs after the code which immediately follows it. That sounds at least somewhat plausible.

There's also consistency with Dispatcher to consider, since you're always going to get a shift there by definition and we can't change that semantic. My guess is this is a bigger problem in practice than IO's own runners.

[vasilmkd]

That's maybe workable though but it starts to get ugly. It's also really hard for me to say whether or not someone has written code which depends on this semantic on JS.

What about this #2530 (comment) and this #2530 (comment). This is library code.

[djspiewak]

@vasilmkd I took a look in scalajs-react and, while I didn't find onClick specifically, it does seem based on how things are being wired up that these cases just don't work inside scalajs-react today. It seems to rely on unsafeRunAndForget and unsafeRunAsync exclusively.

[vasilmkd]

I did try tagging @japgolly on the original issue, but without success.

[armanbilge]

David (japgolly) is less available these days. FWIW I don't really see this issue as specific to scalajs-react, it's just caught in the middle. But maybe I'm missing something.

[vasilmkd]

It's not specific, but it's a successful and well used library which implicitly depends on this behavior (at least on first look), so it is an argument for offering some configuration or making more fundamental changes to the execution behavior.

[djspiewak]

I feel like we need a broader conversation with Scala.js folks. A lot of this comes down to what is even expected here.

[armanbilge]

I'm generally supporting that, but also I think they have been expressing their expectations in #1862 and #2530 :)

[djspiewak]

Fair but it's not clear to me that everyone involved understands the tradeoffs. All of these issues seem just focused on the most visible current outcome.

[armanbilge]

That includes me I guess. What is the tradeoff for avoiding that initial shift?

[djspiewak]

What is the tradeoff for avoiding that initial shift?

For starters, Dispatcher can't avoid that initial shift. But let's just focus on IO for a moment and pretend that we're okay with making the two inconsistent:

foo()
IO(bar()).unsafeRunAndForget()
baz()

Does bar happen before or after baz? Right now, it's after. Avoiding the shift would mean that it happens before. This has implications for correctness, since people may be assuming that baz() happens first due the asynchrony involved, and it also has implications for fairness and a few other things. What if bar() takes a really long time? What if the author assumed that we're yielding to the event loop before hand? We do on the JVM!

The correctness one is worth dwelling on as well. As you pointed out, people are coming to us right now complaining that bar isn't happening before baz, but is that only because that's how it happens to work right now? If we changed it to bar happening before baz (by removing the yield), how many people who are content with the current semantics would start reporting issues complaining that bar is happening before baz? That's very hard to say, and the choice is somewhat arbitrary.

Or rather, it would be arbitrary, except for the fact that Dispatcher on all platforms and all-things-JVM must insert a yield at the beginning. So we've basically chosen the arbitrary path which is consistent with our other constraints so that we only have one semantic to explain, and not two, and particularly not two for the same function (e.g. unsafeRunAndForget) across different platforms.

I really think the right answer is to lean into syncStep. An unyielding { ... } construct would also be very interesting, and maybe we should explore that (it also has applications on the JVM), but that doesn't address the initial yield.

[armanbilge]

Thanks for the detailed response! Yes please, let's focus on IO for the moment. TBH I haven't yet sat down with Dispatcher to understand why we can't change the semantics there (I don't want to believe it!). For now, here are some assorted thoughts.

I do see the confusion with your snippet, but I'm not sure how realistic it is. I genuinely believe users want to adopt CE3/IO whole-heartedly (and we should be encouraging it!) and as such that code would more likely be written like this:

(IO(foo()) *> IO(bar()).start *> IO(baz())).unsafeRunAndForget()

Once it's written like this, I think reasoning about this code becomes much easier. I'm not read-up enough to say for sure if this makes sense, but: I think the idea is to rely more heavily on laws (i.e., replace IO with F) rather than specific implementation details.

That's very hard to say, and the choice is somewhat arbitrary.

Well, sure. Except with the current semantics, everyone gets stuck with the shift. However, if we remove that, it should be possible to explicitly opt-in with e.g. (IO.cede *> whatever).unsafeRunAndForget(), right? Best of both worlds?

so that we only have one semantic to explain, and not two, and particularly not two for the same function (e.g. unsafeRunAndForget) across different platforms.

I absolutely agree that having two semantics for the same method is pretty terrible. My best remark about this is that if we (perhaps unrealistically) assume a purely-CE cross-platform-Scala stack, the only place users should have to work with these unsafe methods is when they are calling into JVM-only or JavaScript-only APIs. In which case, that code is most certainly not cross-platform anyway, and it gives us some leeway for different semantics.

But ... I'm still not entirely convinced that the semantics are different here. Let's look back at this snippet:

foo()
IO(bar()).unsafeRunAndForget()
baz()

Does bar happen before or after baz?

On the JVM, the answer is ... non-determinstic!! So, how would changing the semantics on JS contradict something that is non-determinstic on JVM? Apologies if I'm being foolish here.

An unyielding { ... } construct would also be very interesting, and maybe we should explore that (it also has applications on the JVM), but that doesn't address the initial yield.

💯 to this. Let me spin it out into an issue.

[japgolly]

I did try tagging @japgolly on the original issue, but without success.

Ah sorry, I must've missed it. Would you mind re-sharing your question? A quick ctrl-f here in this issue doesn't turn anything up

[armanbilge]

@japgolly great to see you! @vasilmkd was referring to this comment I believe: #2530 (comment)

[japgolly]

Hi @armanbilge !!! Thanks and you too! You know I drop into to scalajs-dom from time to time to see if there's anything I can do but you're such a force, everything's always already merged! hehe, love it mate 😊

Ah ok cool thanks for the link, i'll reply now

[djspiewak]

@japgolly Tldr, IO inserts a leading yield whenever you call any of the unsafeRuns, meaning that things like preventDefault() seem like they can't possibly work at present within scalajs-react. We were trying to get some clarity on that, and also brainstorm on expected behavior.

[vasilmkd]

@japgolly Welcome to the discussion. I feel like I need to say this, but my previous comment wasn't snarky in any way, nor did it have some hidden meaning. I wasn't expecting anything from you with regards to your time. I really just want your opinion if you would lend it. 😄

Your input as a maintainer of scalajs-react would be very much appreciated in this issue as we are in a unique position to collaborate and enable as many users as possible.

Thank you again.