Only use fibers if caller uses fibers.

[kentonv]

Currently (without this change), callbacks passed to .then() will always run in a fiber when this package is active.

This is useful for Meteor code where many things must run in fibers. However, it is problematic for non-Meteor code that doesn't expect fibers. The main problem is performance: creating and switching fibers has a cost. Fiber-switching in particular trashes the cache.

Traditionally, promise-heavy code (that doesn't expect fibers) tends to involve lots of chaining of short continuations using .then(). Unfortunately, this style maximizes the overhead of creating a fiber for every callback.

Code that expects fibers can reduce the number of fibers created by taking advantage of them, e.g. by using .await() rather than chaining. But code which is not Meteor-specific cannot assume fibers.

The result is that promise-heavy code which happens to live in the same process as Meteor code will suffer severe performance penalties. Because the global definition of "then" is being overridden, it applies throughout the process, even to code that doesn't need or want fibers.

The best solution to this would have been never to have introduced this monkey-patch in the first place. Instead, a new method, e.g. thenInFiber(), should have been introduced.

However, it is probably too late for that now.

The opposite solution does not work: we cannot introduce a thenNoFiber() method because code which wasn't written for Meteor in the first place cannot be expected to use this.

As a compromise, this patch implements the rule: If the code which called .then() is in a fiber, then the continuations will be run in a fiber. If not, they won't.

This should have minimal impact on any Meteor code which expects .then() to run in a fiber, because the caller will also normally be in a fiber. Meanwhile, non-Meteor code can be kept out of fibers as long as developers are careful to schedule it outside of any fiber in the first place.

I've also added the methods thenInFiber() and thenNotInFiber() to help high-level code be explicit about pushing things in and out of fibers.

There's some chance this change will break existing code which explicitly expected .then() to add fibers when the caller wasn't in a fiber. I can't see a good way to solve the problem here without such breakage. Hopefully it is rare.

Fixes #9

[kentonv]

Unlike #11, this is not urgent, and should be treated with caution since it could cause breakages. However, it's important to us at Sandstorm and possibly to others in similar situations that this gets fixed, and I don't see a way to fix it without possibly introducing this breakage.

Thoughts?

[benjamn]

This is an interesting idea and definitely worth exploring, though I can think of a number of places where the existing behavior is relied upon to ensure callback code runs in fiber, even though .then was not called in a fiber.

Another approach might be to reduce the cost by somehow not always switching fibers, though I'm not entirely sure the details will make sense.

Note: I'm just getting back from vacation today, and have a lot of catching up to do. If I don't respond in more detail in a few days, feel free to ping me again!

[kentonv]

Hmm. You could maybe do something like `s/.then(/.thenInFiber(/g' across the whole Meteor codebase to fix all call sites within Meteor without having to analyze each one.

How many breakages do you expect to see in apps and third-party packages? If a lot, maybe an opt-out would make sense: don't change the default behavior, but give Sandstorm a way to opt out of automatic fibers without changing behavior for other apps. This still requires that all of Meteor (and any other libraries Sandstorm uses) works either way -- and it might be tricky to make sure new violations aren't introduced over time.

[kentonv]

@benjamn My friends on the ecmascript committee have pointed out that this monkey-patch actually introduces an important violation of the Promise spec: It is considered central to Promises that two then-callbacks cannot interleave, because this makes it much easier for programmers to reason about possible interference between callbacks.

I really think that Meteor needs to think seriously about undoing this monkey-patch, ideally sooner rather than later since the longer it exists, the more things may grow dependent on it.

[benjamn]

Those people (I know them too; I'm on the TC39 committee) disagree more fundamentally with the use of fibers, since Fiber.yield is the only reason two .then callbacks could ever interleave.

The interleaving hazard is worth understanding, but hardly unique to JavaScript, since many other languages have coroutines and/or threads. This library makes it strictly easier to work with fibers, since you don't have to worry about creating/recycling Fiber objects, forgetting to call .run() after Fiber.yield(), or sharing Fiber-local state between fibers. Thanks to this library, the Promise API becomes the only interface you need to manipulate fibers, and the more you use Promises without thinking about fibers, the less important fibers will become.

I'm aligned with the longer-term idea of transitioning Meteor code (both the command-line tool and third-party app and package code) to use async/await instead of fibers. The main challenge with that transition is finding all the callers of a function that you've changed from using fibers to returning a Promise, and properly awaiting or .thening the result. And of course the calling function probably needs to return a Promise, too, and likewise for every caller of that function, and so on.

We're not going to be introducing any new APIs that rely on fibers, but as long as Meteor contains APIs that need to yield, I don't see any alternative to a library like this one.

[kentonv]

I don't see any alternative to a library like this one.

Isn't .thenInFiber() a reasonable alternative? I certainly don't expect Meteor to move away from fibers any time soon. We just need it not to impose fibers on unrelated code running in the same process...

[jimrandomh]

After several days tracking down catastrophically bad SSR performance in a production application, I've ended up here. I believe this is the root of a lot of serious problems our site (lesswrong.com) has had, and I'm quite bothered by the fact that this issue has apparently been sitting known-and-neglected for two years.

[jimrandomh]

Oh, ugh... somehow we've ended up with multiple libraries providing promise polyfills. I think I need to investigate a bit more before firmly casting the blame on this issue.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[CLAassistant]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[StorytellerCZ]

Any updates on this?

[StorytellerCZ]

I'm closing this issue because we haven't heard anything from the original poster for a while.

[kentonv]

@StorytellerCZ Hi, I'm the original poster. This PR won't work as described earlier -- apparently, many things depend on .then() callbacks running in a fiber even if the caller of .then() wasn't. So, closing it makes sense. But, the performance problem remains -- spawning a fiber for every continuation is quite expensive, making promise-based code run more slowly when called from a Meteor app. I guess it's up to the Meteor team whether you want to do anything about it.

At least, that was the state of the world five years ago when I made this PR, I don't know if anything fundamental has changed.

[StorytellerCZ]

Thanks you @kentonv for you comment. Fibers are going away in the near future: https://github.com/meteor/meteor/discussions/11505 primarily due to the fact that Fibers have been deprecated and won't work from Node 16. Still this is something that is going to take a while and will take some time for people to update, so any improvements in the meantime are welcome if you want to re-work it for the current code we will be happy to take a look on it.

[kentonv]

Oh my, that's going to require quite a rewrite of all Meteor apps I suppose. Ouch.

[StorytellerCZ]

@kentonv we are committed to make this as painless as possible. Hopefully the main rewrites are going to be centered on packages instead of apps.