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

Update feature flag for stack switching proposal #436

Closed
wants to merge 1 commit into from

Conversation

pufferfish101007
Copy link

It seems that the v8 flag listed on https://webassembly.org/roadmap for the stack switching proposal is incorrect; the --experimental-wasm-stack-switching flag exists, but it seems to be unused (note that all references to stack_switching flags are different v8 flags relating to jspi [I think]). However, the --experimental-wasm-wasmfx flag has been added recently, with an incomplete implementation (not yet landed in stable chrome as far as I can tell).

#400 says that there was an in-progress implementation of stack switching back in September, but as far as I can tell this isn't correct?

This pull request updates the listed feature flags for the stack switching proposal... it's difficult to test these because the change was made recently enough to not be in stable releases yet, but hopefully it's correct now. (And hopefully I haven't completely misunderstood the whole situation.)

@tomayac
Copy link
Collaborator

tomayac commented Mar 17, 2025

Thanks for the PR, @pufferfish101007! @tlively Do you know who from V8 would be a good person to verify this? Likely @fgmccabe!?

@fgmccabe
Copy link

The experimental-wasm-stack-switching was the one originally used to enable JSPI. We implemented the experimental-wasm-jspi flag to avoid confusion; and we kept the original flag as a synonym.
The experimental-wasm-wasmfx flag is there to guard the prototype implementation of core stack switching. Very much a 'work in progress' at the moment.

@Liedtke
Copy link
Contributor

Liedtke commented Mar 17, 2025

@fgmccabe Note that the flag is documented to be used for the stack switching proposal though:

  /* Stack Switching proposal. */                                              \
  /* https://github.com/WebAssembly/stack-switching */                         \
  /* V8 side owner: thibaudm, fgm */                                           \
  V(stack_switching, "stack switching", false)                                 \

Should we update the comment then to avoid further confusion? (I was confused by this as well a few weeks ago.)

@fgmccabe
Copy link

We could. Not sure it is worth it though:
We are pretty close now to shipping JSPI.
Originally, it seemed likely that we would have our own design proposal, and having a separate flag for that seemed useful. Now, that scenario is unlikely: there is a unified proposal that is based on the wasm-fx proposal.
At some point after shipping JSPI we will be removing the JSPI flag; we can remove wasm-stack-switching at the same time.

@tomayac
Copy link
Collaborator

tomayac commented Mar 27, 2025

What's the verdict here? Should we close this PR for now?

@Liedtke
Copy link
Contributor

Liedtke commented Mar 27, 2025

We should at least remove the mention that --experimental-wasm-stack-switching controls the stack switching proposal as it doesn't. If "incomplete" features behind a flag should display their flag, I guess, this PR can just be merged, otherwise we should probably update it to "Chrome/V8 doesn't support stack switching yet."

@tomayac
Copy link
Collaborator

tomayac commented Mar 27, 2025

@Liedtke Could you make the change, as you're close? Happy to merge once you tell me. Thanks!

@Liedtke
Copy link
Contributor

Liedtke commented Mar 27, 2025

@tomayac Sure, I assume that means the right thing is to remove the flag mention for all V8-based implementations for now: #444

@tomayac
Copy link
Collaborator

tomayac commented Mar 27, 2025

#444 is merged now, so what do we do with this present PR?

If "incomplete" features behind a flag should display their flag, I guess, this PR can just be merged

So, can it? (Sorry, I'm not involved enough to have an answer.)

@tomayac
Copy link
Collaborator

tomayac commented Mar 27, 2025

We also have a conflict now of course…

@Liedtke
Copy link
Contributor

Liedtke commented Mar 27, 2025

So, can it? (Sorry, I'm not involved enough to have an answer.)

Well, maybe that was the misunderstanding, based on your request for a PR I interpreted it as "it doesn't make sense to display 'this feature exists behind the following flag' if you can't do anything useful when setting that flag."

Probably we want to "publish" feature flags here once they are in a state where you can start experimenting with them? (Which wouldn't require having the full proposal implemented but at least some part that is usable and testable.)

@tomayac
Copy link
Collaborator

tomayac commented Mar 27, 2025

Alright, so closing this PR then. Thank you!

@tomayac tomayac closed this Mar 27, 2025
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

Successfully merging this pull request may close these issues.

4 participants