Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Top-level await silently reordering evaluation order seems bad #42

Open
syg opened this issue Apr 9, 2024 · 4 comments
Open

Top-level await silently reordering evaluation order seems bad #42

syg opened this issue Apr 9, 2024 · 4 comments

Comments

@syg
Copy link

syg commented Apr 9, 2024

I'm still not quite understanding why it's acceptable to have TLA silently change the evaluation order.

If you import defer a module graph, subgraphs that contain TLA are eagerly evaluated. So, when a dependency deep down in the tree gets (or removes!) a TLA, the set of modules that get their toplevels deferred silently changes. This seems like very surprising behavior to me.

One counterargument I've heard is that these dependencies are out of the importer's control, so if, for example, TLA subgraphs caused import defer to throw instead of eager evaluation, then import defer would just break. But from my perspective breaking seems more desirable. If you are using import defer to move some set of toplevel evaluations later in time, would you not be interested in when that set of toplevels changes? This is changing observable behavior instead of a behavior-preserving optimization.

@guybedford
Copy link
Collaborator

The intent of import defer is to get the module ready for synchronous evaluation. We could have excluded TLA and only permitted synchronous graphs, but it seems problematic conversely to exclude valid cases where there's a small low-level TLA dependency, while there can be perf benefits to be found for import defer even in these cases. @nicolo-ribaudo did some amazing work here formulating a very elegant solution to subgraph evaluation that solves this use case without over-complicating the spec algorithms.

In terms of evaluation ordering under TLA, as I mentioned in plenary, TLA executions can always be raced by another top-level importer, so I struggle to see how this is a "reordering".

From the consumer perspective, adding defer to an import is a "blind" performance optimization. import defer doesn't report anything about what is executing and in what order to its users. And since it will be used alongside normal imports, it's not like there aren't executions already being queued synchronously. Having it sometimes work and sometimes not seems more likely to be the randomly frustrating path IMO.

I can however appreciate that it might not be clear to users that defer might perform execution. The hazard here being "false advertising" as opposed to determinism. But I don't feel that ordering determinism is in anyway invalidated in terms of how and when the dependencies themselves expect to be executed within the application lifecycle.

@syg
Copy link
Author

syg commented Apr 9, 2024

In terms of evaluation ordering under TLA, as I mentioned in plenary, TLA executions can always be raced by another top-level importer, so I struggle to see how this is a "reordering".

With TLA and without import defer, a module toplevel can race witho another module toplevel. With import defer, a module toplevel's execution can be interleaved with arbitrary other user code (e.g. an event handler). The hazard I'm concerned about is that whether a toplevel is interleaved or not depends on the presence of TLA in some module subgraph, which is hard to discern.

For a concrete example, suppose I update the deps, and one of my deps removed a TLA. I import defer this module and use its namespace object inside an event handler. The "reordering" is that before I updated my deps, my module's toplevel was not interleaved with the event handler code. With the removal of TLA, now it is. This change in behavior is silent and hard to figure out from my own code, since it was my dep that changed.

From the consumer perspective, adding defer to an import is a "blind" performance optimization. import defer doesn't report anything about what is executing and in what order to its users.

My concern is that it seems bad to bill something that can change when a module toplevel executes as a blind performance optimization. I can live with if the developer has to figure out once, at the point when the developer decides to change an import to an import defer, if the changed module toplevel execution works for their app. I have a harder time living with that they have to figure it out continually, each time their dependencies change.

Having it sometimes work and sometimes not seems more likely to be the randomly frustrating path IMO.

This seems to be the crux of the disagreement. Why is this more frustrating than silently reordering top-level executions depending on addition/removal of TLA? The latter seems more frustrating.

I can however appreciate that it might not be clear to users that defer might perform execution.

I hope that some deferred execution happens is clear, since that's the whole point of the proposal. It's that what gets deferred is hard to know and can change when your dependencies changed.

@guybedford
Copy link
Collaborator

guybedford commented Apr 9, 2024

For a concrete example, suppose I update the deps, and one of my deps removed a TLA. I import defer this module and use its namespace object inside an event handler. The "reordering" is that before I updated my deps, my module's toplevel was not interleaved with the event handler code. With the removal of TLA, now it is. This change in behavior is silent and hard to figure out from my own code, since it was my dep that changed.

This seems less about TLA and more about the execution semantics of defer itself in that it represents a new top-level evaluation which can happen at arbitrary positions. It's also the primary feature of the proposal is the tough part here. One way to think of it is that there's an import.sync(mod) at the position of every deferred namespace access.

The point being it is an equal top-level importer like any other, and is subject to the determinism of all top-level importers when racing eachother. require(esm) in Node.js is also very semantically similar as well.

I have a harder time living with that they have to figure it out continually, each time their dependencies change.

Again, I think this is a fact of defer, not its TLA behaviour. If the dependencies change, the error at the call site of the deferred access changes.

Why is this more frustrating than silently reordering top-level executions depending on addition/removal of TLA?

Working code is always less frustrating than code that never works. And I think you're overestimating how much dependency code might fail due to top-level initialization ordering changes that are already subject to random execution determinism and hence libraries have already factored this variation in, versus a user's expectation for a performance feature.

I hope that some deferred execution happens is clear, since that's the whole point of the proposal. It's that what gets deferred is hard to know and can change when your dependencies changed.

My point is that it might not be clear to users that if you do:

import defer * as x from './x.js';
// <nothing else>

that the above can execute code. That would be a much more compelling argument for why TLA is an issue to me.

@acutmore
Copy link
Contributor

But from my perspective breaking seems more desirable. If you are using import defer to move some set of toplevel evaluations later in time, would you not be interested in when that set of toplevels changes?

The issue with TLA throwing at runtime is that there is not a way to acknowledge the break, analysis it and then update the code to continue to opt in now satisfied that the TLA change is acceptable.

Even with TLA aside there are other changes that apps will be interested in. For example:

// app.js
import * as a from "a.js";
import * as b from "b.js";
...

// a.js
import defer * as lib from "lib";
...

// b.js
export const x = 1;

All is well, lib is deferred.

But if someone changes b to:

import * as lib from "lib";

export const x = 1;

export function f() {
  return lib.f();
}

This has de-optimised a's deferred import.

Our developer tooling at Bloomberg looks for this pattern and can warn the developer that they have a lazy import that is "shadowed" by an eager one. So they can remedy the situation. This can be a CI check for example.

Both this and the introduction of TLA I think are the job for tooling and performance monitoring to catch. Rather than make them immovable barriers.

The defer keyword is the opt-in request saying that it is okay to change the behaviour in an observable way. Maybe a different keyword would make it clearer that it's a request and not a demand.

A design like:

performAsyncWork "lib.js";

...later
function f() {
  const lib = import.sync("lib.js");
  return lib.f();
}

Is effectively the mental model but the module being ready for the sync import is dependent on that top level static request to perform the async work. This coupling motivates a design that clarifies the connection by getting the deferred namespace as part of the preparation.

performOnlyAsyncWork * as lib from "lib";
deferSyncWork * as lib from "lib";
import defer * as lib from "lib";

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants