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

Editorial: Explicitly track async evaluation order of pending modules #3353

Merged
merged 1 commit into from
Mar 6, 2025

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jun 14, 2024

The execution order of modules that were waiting on a given async module was based on the order in which their [[AsyncEvaluation]] field was set to true.

This PR replaces the [[AsyncEvaluation]] boolean with an actual number that keeps track of ordering in each agent. It also adds a note on when it's safe to reset this order to 0, since:

  • implementations might want to prevent this number from overflowing, and V8 currently has a bug that resets the number to 0 too early
  • figuring out when it's safe to reset the number might not be trivial, given the complexity of the module algorithms.

You can verify the numbers in the updated tables for the example using https://nicolo-ribaudo.github.io/es-module-evaluation/.

Fixes #3289

@nicolo-ribaudo nicolo-ribaudo changed the title Explicitly track async evaluation order of pending modules Editorial: Explicitly track async evaluation order of pending modules Jun 14, 2024
@nicolo-ribaudo nicolo-ribaudo force-pushed the async-evaluation-order branch 2 times, most recently from d600852 to f96a304 Compare June 14, 2024 22:20
@michaelficarra michaelficarra requested a review from syg June 17, 2024 17:47
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks for making this clearer.

As you may have seen in #3356, there's a bug in this machinery. The fix is a one liner so we should fix that bug, and you'll need to rebase this on top of the fix.

@nicolo-ribaudo nicolo-ribaudo force-pushed the async-evaluation-order branch 2 times, most recently from 4cdd981 to b1c782e Compare February 21, 2025 19:36
@nicolo-ribaudo
Copy link
Member Author

PR updated, sorry I forgot about this.

For #3353 (comment), I pushed it in a second commit to make it easier to review where I'm using ~unset~ and where I'm using ~done~. I added a few assertions to make it easier.

Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@bakkot bakkot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with my comment. This is so much nicer, thanks.

Don't forget to update https://tc39.es/proposal-defer-import-eval/#sec-example-cyclic-module-record-graphs-deferred-imports after this lands.

jmdyck
jmdyck previously requested changes Feb 22, 2025
Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise. I'm just trusting that the numbers in the "Example Cyclic Module Record Graphs" table are correct.

@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Feb 26, 2025
@bakkot
Copy link
Contributor

bakkot commented Mar 5, 2025

Ready except it needs the ~sync~ enum value changed in the last commit reverted back to ~unset~ (but not the other uses of the ~sync~ value which predate this PR).

Edit: I went ahead and did this myself.

@bakkot bakkot added ready to merge Editors believe this PR needs no further reviews, and is ready to land. and removed editor call to be discussed in the next editor call labels Mar 5, 2025
…tc39#3353)

The execution order of modules that were waiting on a given async module was based on the order in which their [[AsyncEvaluation]] field was set to *true*.

This PR replaces the [[AsyncEvaluation]] boolean with an actual number that keeps track of ordering in each agent.
It also adds a note on when it's safe to reset this order to 0, since:
 - implementations might want to prevent this number from overflowing, and V8 currently has a bug that resets the number to 0 too early
 - figuring out when it's safe to reset the number might not be trivial, given the complexity of the module algorithms.
@ljharb ljharb force-pushed the async-evaluation-order branch from ca9388f to 030dcd6 Compare March 6, 2025 15:47
@ljharb ljharb dismissed jmdyck’s stale review March 6, 2025 15:47

changes addressed

@ljharb ljharb merged commit 030dcd6 into tc39:main Mar 6, 2025
7 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the async-evaluation-order branch March 6, 2025 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
6 participants