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

Consider enforcing --experimental-worker flag until Atomics.wake is renamed Atomics.notify #21219

Closed
rwaldron opened this issue Jun 8, 2018 · 16 comments
Labels
worker Issues and PRs related to Worker support.

Comments

@rwaldron
Copy link

rwaldron commented Jun 8, 2018

At the May TC39 meeting, I proposed renaming Atomics.wake to Atomics.notify in order to better differentiate it from Atomics.wait: Renaming Atomics.wake (please review before continuing). With complete support from implementers, the committee agreed and set forth "next steps".

The "needs consensus" PR will not be up for approval until late July. If Node ships the new Worker API without a flag before that approval, developers will inevitably write programs that rely on Atomics.wake and the argument on which I predicated this opportunity for improvement will be struck down, ie. that no new code featuring Atomics.wake has been written since the Spectre/Meltdown mitigations.

Can we work together on this? That would be ❤️ 👍

@rwaldron
Copy link
Author

rwaldron commented Jun 8, 2018

cc @addaleax

@addaleax addaleax added the worker Issues and PRs related to Worker support. label Jun 8, 2018
@devsnek
Copy link
Member

devsnek commented Jun 8, 2018

this seems reasonable, I'm always a fan of helping out tc39. I doubt we would have unflagged workers by then anyway.

@addaleax
Copy link
Member

addaleax commented Jun 8, 2018

I’m totally on board with the rename.

Once the decision for a name is made, what I’d see Node doing is providing the new name immediately, and providing Atomics.wake (with a deprecation warning either immediately or in the next major version). How exactly that looks is probably going to depend on how V8 ends up implementing the name change.

The "needs consensus" PR will not be up for approval until late July. If Node ships the new Worker API without a flag before that approval.

I don’t think that’s realistic, it’s going to take (possibly a lot) longer than that. If nothing else, we can always block that on this issue.

@leobalter
Copy link
Contributor

As someone working on the current renaming proposal, I appreciate your support @devsnek @addaleax!

@benjamingr
Copy link
Member

Hey @rwaldron @leobalter - from another angle it would be absolutely fantastic if you took --experimental-worker for a spin and give Node criticism about how it works and whether or not it aligns with your vision.

Note that even if we do ship workers unflagged (which I think we won't, now that you've asked and Anna and Gus both agreed and I agree with them) - they'd still be experimental and we'd still be fine with "breaking" Atomics.wake.

@rwaldron
Copy link
Author

@benjamingr will do!

@rwaldron
Copy link
Author

Once the decision for a name is made

This has already been agreed to: Atomics.notify

@addaleax
Copy link
Member

@rwaldron Does that mean that we could introduce that alias in Node right now? Or is it “just” the fixed name in the proposal and TC39 has yet to decide whether it’s going to be merged in the spec that way?

@rwaldron
Copy link
Author

@addaleax sorry for the delay responding...

Does that mean that we could introduce that alias in Node right now?

That would be ideal, yes.

Or is it “just” the fixed name in the proposal and TC39 has yet to decide whether it’s going to be merged in the spec that way?

The change has consensus, but was made subject to the "consensus PR" process.

@devsnek
Copy link
Member

devsnek commented Jun 19, 2018

is this all that is needed? do we need to worry about function.name?

Object.defineProperty(Atomics, 'notify', {
  value: Atomics.wake,
  writable: true,
  enumerable: false,
  configurable: true,
});

@addaleax
Copy link
Member

@devsnek I assume we want to do this in NewContext() in node.cc to make sure it’s available that way in VM contexts as well

@rwaldron
Copy link
Author

is this all that is needed?

Yes, that works 👍

do we need to worry about function.name?

My nitpicking spec conformance self says yes, but my pragmatic self says no ;)

@addaleax
Copy link
Member

@devsnek do you want to do this? if not, i’ll put something together today or tomorrow

@devsnek
Copy link
Member

devsnek commented Jun 19, 2018

@addaleax yeah I can take it.

devsnek added a commit that referenced this issue Jun 21, 2018
PR-URL: #21413
Refs: #21219
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Jun 22, 2018
PR-URL: #21413
Refs: #21219
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented Nov 13, 2018

Should this be closed now that there's an alias?

@targos
Copy link
Member

targos commented Nov 18, 2018

I think it can. We have Atomics.notify() and Atomics.wake() emits a warning.

@targos targos closed this as completed Nov 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
worker Issues and PRs related to Worker support.
Projects
None yet
Development

No branches or pull requests

7 participants