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

src: add Atomics.notify alias #21413

Closed
wants to merge 1 commit into from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Jun 19, 2018

Refs: #21219

/cc @rwaldron

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@devsnek devsnek requested a review from addaleax June 19, 2018 21:25
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. tools Issues and PRs related to the tools directory. labels Jun 19, 2018

const { strictEqual } = require('assert');

strictEqual(Atomics.notify, Atomics.wake);
Copy link
Member

Choose a reason for hiding this comment

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

maybe add a test for vm as well?

@addaleax addaleax added the worker Issues and PRs related to Worker support. label Jun 19, 2018
@devsnek devsnek force-pushed the feature/atomics-notify branch from ee6efd2 to f99a3ea Compare June 19, 2018 21:31
@leobalter
Copy link
Contributor

To prevent any sort of hazard, I wonder if Atomics.notify should be preserved if it already exists from V8, this way it's not necessary to clone anything from Atomics.wake.

@devsnek
Copy link
Member Author

devsnek commented Jun 19, 2018

@leobalter when we update v8 to a version that has Atomics.notify we can just remove this change. i would prefer that actually so we don't have dead code hanging around.

@leobalter
Copy link
Contributor

@devsnek I'm a newbie wrt node ecosystem, if this can be done, I'm +1. Having Atomics.notify here is great anyway.

@joyeecheung
Copy link
Member

Does this actually help with the issue raised in #21219 if Atomics.wake is still available?

that no new code featuring Atomics.wake has been written since the Spectre/Meltdown mitigations.

At least if I go to MDN to look for docs I'll still see Atomics.wake and nothing about Atomics.notify so if I were not aware of that issue I'll still use Atomics.wake. Maybe a warning for accessing Atomics.wake could help, but I am not sure if we step over the line regarding tampering with ECMAScript features if we do that

context->SetEmbedderData(
ContextEmbedderIndex::kAllowWasmCodeGeneration, True(isolate));

auto intl_key = FIXED_ONE_BYTE_STRING(isolate, "Intl");
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason these lines got moved?

Copy link
Member Author

Choose a reason for hiding this comment

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

just a side-effect of the changes i was making before i finished up. should i change it back? they kinda make the order better anyway.

@devsnek
Copy link
Member Author

devsnek commented Jun 20, 2018

@joyeecheung i wanted to add warnings and stuff but since we have to do it in c++ i don't know how viable it is. maybe we could move the intl and atomics stuff into a js file somewhere that's only called when making a new context. iirc v8_extras also run per-context so those could help too.

@devsnek
Copy link
Member Author

devsnek commented Jun 20, 2018

@nodejs/v8 out of curiosity i tried to implement this with v8 extras but any attempt to access or set properties on global.Atomics results in mksnapshot failing. is this a bug or something else? i can't find any other global objects where messing with them fails.

@hashseed
Copy link
Member

Do you have any backtrace from a debug build? I suspect that some external references are missing.

@devsnek
Copy link
Member Author

devsnek commented Jun 20, 2018

@hashseed i don't have the power atm to fully recompile with debug but i've got

  LD_LIBRARY_PATH=/Users/gus/Desktop/misc/nodejs/node/out/Release/lib.host:/Users/gus/Desktop/misc/nodejs/node/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../deps/v8/gypfiles; mkdir -p /Users/gus/Desktop/misc/nodejs/node/out/Release/obj.target/v8_snapshot/geni; "/Users/gus/Desktop/misc/nodejs/node/out/Release/mksnapshot" --startup_src "/Users/gus/Desktop/misc/nodejs/node/out/Release/obj.target/v8_snapshot/geni/snapshot.cc" ""
/bin/sh: line 1: 54182 Segmentation fault: 11  "/Users/gus/Desktop/misc/nodejs/node/out/Release/mksnapshot" --startup_src "/Users/gus/Desktop/misc/nodejs/node/out/Release/obj.target/v8_snapshot/geni/snapshot.cc" ""
make[1]: *** [/Users/gus/Desktop/misc/nodejs/node/out/Release/obj.target/v8_snapshot/geni/snapshot.cc] Error 139

also running mksnapshot (which iirc should show a help message?):

(lldb) run
Process 54499 launched: 'node/out/Release/mksnapshot' (x86_64)
mksnapshot was compiled with optimization - stepping may behave oddly; variables may not be available.
Process 54499 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x0000000100007928 mksnapshot`v8::SnapshotCreator::SetDefaultContext(v8::Local<v8::Context>, v8::SerializeInternalFieldsCallback) [inlined] v8::internal::MemoryChunk::FromAddress(a=<unavailable>) at spaces.h:410 [opt]
   407
   408 	  // Only works if the pointer is in the first kPageSize of the MemoryChunk.
   409 	  static MemoryChunk* FromAddress(Address a) {
-> 410 	    return reinterpret_cast<MemoryChunk*>(OffsetFrom(a) & ~kAlignmentMask);
   411 	  }
   412
   413 	  static inline MemoryChunk* FromAnyPointerAddress(Heap* heap, Address addr);
Target 0: (mksnapshot) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000100007928 mksnapshot`v8::SnapshotCreator::SetDefaultContext(v8::Local<v8::Context>, v8::SerializeInternalFieldsCallback) [inlined] v8::internal::MemoryChunk::FromAddress(a=<unavailable>) at spaces.h:410 [opt]
    frame #1: 0x0000000100007921 mksnapshot`v8::SnapshotCreator::SetDefaultContext(v8::Local<v8::Context>, v8::SerializeInternalFieldsCallback) [inlined] v8::internal::HeapObject::GetHeap(this=<unavailable>) const at objects-inl.h:957 [opt]
    frame #2: 0x0000000100007921 mksnapshot`v8::SnapshotCreator::SetDefaultContext(v8::Local<v8::Context>, v8::SerializeInternalFieldsCallback) [inlined] v8::internal::HeapObject::GetIsolate(this=<unavailable>) const at objects-inl.h:966 [opt]
    frame #3: 0x0000000100007921 mksnapshot`v8::SnapshotCreator::SetDefaultContext(v8::Local<v8::Context>, v8::SerializeInternalFieldsCallback) [inlined] v8::Context::GetIsolate(this=0x0000000000000000) at api.cc:6432 [opt]
    frame #4: 0x0000000100007921 mksnapshot`v8::SnapshotCreator::SetDefaultContext(this=0x00007ffeefbff758, context=<unavailable>, callback=SerializeInternalFieldsCallback @ 0x00007f99804143c0) at api.cc:583 [opt]
    frame #5: 0x0000000100001995 mksnapshot`main at mksnapshot.cc:299 [opt]
    frame #6: 0x000000010000192b mksnapshot`main(argc=1, argv=<unavailable>) at mksnapshot.cc:409 [opt]
    frame #7: 0x00000001000017e4 mksnapshot`start + 52
(lldb) ^D

@hashseed
Copy link
Member

I think you introduced some code that caused bootstrapping (creating a context from scratch) to fail. It then returns an empty handle, so that using it as a default context in the snapshot causes a crash.

@devsnek
Copy link
Member Author

devsnek commented Jun 20, 2018

@hashseed here's the script but the repro is as simple as (function (global, binding, v8) { global.Atomics.wake; })

https://gist.github.com/devsnek/3679af4029594f31a62b70e35ed035d9

@hashseed
Copy link
Member

That's because Atomics are an experimental feature and not available for contexts intended for a snapshot.

@devsnek
Copy link
Member Author

devsnek commented Jun 20, 2018

@hashseed is that the kind of thing where a flag can be flipped or do we just have to wait and use another solution for now.

@hashseed
Copy link
Member

I don't think there is a good solution currently. By default, V8 ships without Atomics right now. Requiring them to create a snapshot fails as intended. Flipping --harmony-sharedarraybuffer does not help because we don't enable any experimental features to create the snapshot even if they are enabled by default. That way you preserve the ability to disable them at runtime.

So at this point adding it to the snapshot is not really an option, unfortunately.

@devsnek
Copy link
Member Author

devsnek commented Jun 20, 2018

@joyeecheung @addaleax @hashseed this works but is it a good idea? i'm having trouble weighting like the pros and cons here. the diff below lets us set the function name and warn and stuff but i don't know if it causes a slow-down to call it all on every new context created, especially since we can't create a cache for it.

diff --git a/lib/internal/per_context.js b/lib/internal/per_context.js
new file mode 100644
index 0000000000..3dce7f188b
--- /dev/null
+++ b/lib/internal/per_context.js
@@ -0,0 +1,52 @@
+'use strict';
+
+// node::NewContext calls this script
+
+(function(global, binding, v8) {
+  // https://github.com/nodejs/node/issues/14909
+  delete global.Intl.v8BreakIterator;
+
+  // https://github.com/nodejs/node/issues/21219
+  // Adds Atomics.notify and warns on first usage of Atomics.wake
+
+  const AtomicsWake = global.Atomics.wake;
+  const ReflectApply = global.Reflect.apply;
+
+  // wrap for function.name
+  function notify(...args) {
+    return ReflectApply(AtomicsWake, this, args);
+  }
+
+  const warning = 'Atomics.wake will be removed in a future version, ' +
+    'use Atomics.notify instead.';
+
+  let wakeWarned = false;
+  function wake(...args) {
+    if (!wakeWarned) {
+      wakeWarned = true;
+
+      if (global.process !== undefined) {
+        global.process.emitWarning(warning, 'Atomics');
+      } else {
+        global.console.error(`Atomics: ${warning}`);
+      }
+    }
+
+    return ReflectApply(AtomicsWake, this, args);
+  }
+
+  global.Object.defineProperties(global.Atomics, {
+    notify: {
+      value: notify,
+      enumerable: false,
+      configurable: true,
+      writable: true,
+    },
+    wake: {
+      value: wake,
+      enumerable: false,
+      configurable: true,
+      writable: true,
+    },
+  });
+}(this));
diff --git a/node.gyp b/node.gyp
index 301a0e20a1..4f70ee5f29 100644
--- a/node.gyp
+++ b/node.gyp
@@ -24,6 +24,7 @@
     'node_lib_target_name%': 'node_lib',
     'node_intermediate_lib_type%': 'static_library',
     'library_files': [
+      'lib/internal/per_context.js',
       'lib/internal/bootstrap/loaders.js',
       'lib/internal/bootstrap/node.js',
       'lib/async_hooks.js',
diff --git a/src/node.cc b/src/node.cc
index 6cf5f2ad84..70c5a132cc 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -3478,35 +3478,21 @@ Local<Context> NewContext(Isolate* isolate,
   auto context = Context::New(isolate, nullptr, object_template);
   if (context.IsEmpty()) return context;
   HandleScope handle_scope(isolate);
+
   context->SetEmbedderData(
       ContextEmbedderIndex::kAllowWasmCodeGeneration, True(isolate));

-  auto intl_key = FIXED_ONE_BYTE_STRING(isolate, "Intl");
-  auto break_iter_key = FIXED_ONE_BYTE_STRING(isolate, "v8BreakIterator");
-  Local<Value> intl_v;
-  if (context->Global()->Get(context, intl_key).ToLocal(&intl_v) &&
-      intl_v->IsObject()) {
-    Local<Object> intl = intl_v.As<Object>();
-    intl->Delete(context, break_iter_key).FromJust();
-  }
-
-  // https://github.com/nodejs/node/issues/21219
-  // TODO(devsnek): remove when v8 supports Atomics.notify
-  auto atomics_key = FIXED_ONE_BYTE_STRING(isolate, "Atomics");
-  Local<Value> atomics_v;
-  if (context->Global()->Get(context, atomics_key).ToLocal(&atomics_v) &&
-      atomics_v->IsObject()) {
-    Local<Object> atomics = atomics_v.As<Object>();
-    auto wake_key = FIXED_ONE_BYTE_STRING(isolate, "wake");
-
-    Local<Value> wake = atomics->Get(context, wake_key).ToLocalChecked();
-    auto notify_key = FIXED_ONE_BYTE_STRING(isolate, "notify");
-
-    v8::PropertyDescriptor desc(wake, true);
-    desc.set_enumerable(false);
-    desc.set_configurable(true);
-
-    atomics->DefineProperty(context, notify_key, desc).ToChecked();
+  {
+    // Run lib/internal/per_context.js
+    Context::Scope context_scope(context);
+    Local<String> per_context = NodePerContextSource(isolate);
+    v8::ScriptCompiler::Source per_context_src(per_context, nullptr);
+    Local<v8::Script> s = v8::ScriptCompiler::Compile(
+        context,
+        &per_context_src,
+        v8::ScriptCompiler::kNoCompileOptions,
+        v8::ScriptCompiler::kNoCacheBecauseCachingDisabled).ToLocalChecked();
+    s->Run(context).ToLocalChecked();
   }

   return context;
diff --git a/src/node.h b/src/node.h
index 90b2a8cbf6..b551c8bb09 100644
--- a/src/node.h
+++ b/src/node.h
@@ -241,9 +241,7 @@ class MultiIsolatePlatform : public v8::Platform {
 // Creates a new isolate with Node.js-specific settings.
 NODE_EXTERN v8::Isolate* NewIsolate(ArrayBufferAllocator* allocator);

-// Creates a new context with Node.js-specific tweaks.  Currently, it removes
-// the `v8BreakIterator` property from the global `Intl` object if present.
-// See https://github.com/nodejs/node/issues/14909 for more info.
+// Creates a new context with Node.js-specific tweaks.
 NODE_EXTERN v8::Local<v8::Context> NewContext(
     v8::Isolate* isolate,
     v8::Local<v8::ObjectTemplate> object_template =
diff --git a/src/node_javascript.h b/src/node_javascript.h
index d1ad9a2574..00cdc0c0b6 100644
--- a/src/node_javascript.h
+++ b/src/node_javascript.h
@@ -29,6 +29,7 @@
 namespace node {

 void DefineJavaScript(Environment* env, v8::Local<v8::Object> target);
+v8::Local<v8::String> NodePerContextSource(v8::Isolate* isolate);
 v8::Local<v8::String> LoadersBootstrapperSource(Environment* env);
 v8::Local<v8::String> NodeBootstrapperSource(Environment* env);

diff --git a/tools/js2c.py b/tools/js2c.py
index 38cf39f816..249df58085 100755
--- a/tools/js2c.py
+++ b/tools/js2c.py
@@ -189,6 +189,10 @@ namespace {{

 }}  // anonymous namespace

+v8::Local<v8::String> NodePerContextSource(v8::Isolate* isolate) {{
+  return internal_per_context_value.ToStringChecked(isolate);
+}}
+
 v8::Local<v8::String> LoadersBootstrapperSource(Environment* env) {{
   return internal_bootstrap_loaders_value.ToStringChecked(env->isolate());
 }}

@addaleax
Copy link
Member

@devsnek I’d stick to the current approach for this PR

Also, if you want to pursue the extras approach for later, maybe have a look at ayojs/ayo@84fc9ce ? /cc @TimothyGu

@devsnek
Copy link
Member Author

devsnek commented Jun 20, 2018

@devsnek devsnek added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 20, 2018
@devsnek
Copy link
Member Author

devsnek commented Jun 21, 2018

this needs another approval

@rwaldron
Copy link

@joyeecheung I'm on the MDN-PAB and moderate all the JS docs :) I started planning for the Atomics.wake => Atomics.notify docs change/update earlier this week.

@rwaldron
Copy link

rwaldron commented Jun 21, 2018

@hashseed re:

By default, V8 ships without Atomics right now.

I'm not sure I understand this, but that's likely because I missing some important context. If V8 wasn't shipping Atomics by default, and Atomics wasn't available in Node.js, then I wouldn't have any concerns about the urgency of renaming Atomics.wake in the presence of a Worker API, however:

$ nvm use 8
Now using node v8.11.3 (npm v5.6.0)

$ node -e "console.log(process.versions.v8);"
6.2.414.54

$ node -e "console.log(Atomics.wake);"
[Function: wake]

$ nvm use 10
Now using node v10.5.0 (npm v6.1.0)

$ node -e "console.log(process.versions.v8);"
6.7.288.46-node.8

$ node -e "console.log(Atomics.wake);"
[Function: wake]

Flipping --harmony-sharedarraybuffer does not help because we don't enable any experimental features to create the snapshot even if they are enabled by default.

Again, I think I'm missing some important context, so please correct as necessary—but --harmony-sharedarraybuffer is on by default ¯\_(ツ)_/¯

(Also, I noted just now that you were the last person to commit to that file!)

@devsnek
Copy link
Member Author

devsnek commented Jun 21, 2018

@rwaldron so basically since Atomics can be disabled at runtime (i'm not sure on the defaults there) they can't make memory snapshots that use Atomics because it might not exist, in which case the snapshot is invalid. i do agree that i'm confused about atomics being disabled by default, as i think its the other way around.

@hashseed
Copy link
Member

Yup. What @devsnek said.

@rwaldron
Copy link

@devsnek @hashseed thanks for the additional info and your patience!

@devsnek
Copy link
Member Author

devsnek commented Jun 21, 2018

landed in 84f3769

@devsnek devsnek closed this Jun 21, 2018
@devsnek devsnek deleted the feature/atomics-notify branch June 21, 2018 21:24
devsnek added a commit that referenced this pull request Jun 21, 2018
PR-URL: #21413
Refs: #21219
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@devsnek devsnek removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 21, 2018
@targos
Copy link
Member

targos commented Jun 22, 2018

atomis (or atomics) isn't a subsystem of Node. This should have been src: add Atomics.notify alias (not a big deal).

targos pushed a commit that referenced this pull request Jun 22, 2018
PR-URL: #21413
Refs: #21219
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Jul 3, 2018
targos added a commit that referenced this pull request Jul 3, 2018
Notable changes:

* build:
  * Node.js should now be about 60% faster to startup than the previous version,
    thanks to the use V8's code cache feature for core modules. [#21405](#21405)
* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)

PR-URL: #21629
targos added a commit that referenced this pull request Jul 4, 2018
Notable changes:

* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)

PR-URL: #21629
targos added a commit that referenced this pull request Jul 4, 2018
Notable changes:

* dns:
  * An experimental promisified version of the dns module is now available. Give
    it a try with `require('dns').promises`. [#21264](#21264)
* fs:
  * `fs.lchown` has been undeprecated now that libuv supports it. [#21498](#21498)
* lib:
  * `Atomics.wake` is being renamed to `Atomics.notify` in the ECMAScript
    specification ([reference](tc39/ecma262#1220)).
    Since Node.js now has experimental support for worker threads, we are being
    proactive and added a `notify` alias, while emitting a warning if
    `wake` is used. [#21413](#21413) [#21518](#21518)
* n-api:
  * Add API for asynchronous functions. [#17887](#17887)
* util:
  * `util.inspect` is now able to return a result instead of throwing when the
    maximum call stack size is exceeded during inspection. [#20725](#20725)
* vm:
  * Add `script.createCachedData()`. This API replaces the `produceCachedData`
    option of the `Script` constructor that is now deprecated. [#20300](#20300)
* worker:
  * Support for relative paths has been added to the `Worker` constructor. Paths
    are interpreted relative to the current working directory. [#21407](#21407)

PR-URL: #21629
@devsnek devsnek changed the title [atomics] add notify alias src: add Atomics.notify alias Jul 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. tools Issues and PRs related to the tools directory. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet