src: standardise context embedder indices #19135

devsnek · 2018-03-04T21:07:02Z

i'll be adding another one with #19016 so i'd like to keep track of them all a little better

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

TimothyGu

I'd like someone else more familiar with the postmortem stuff to take a look at that part. Otherwise, LGTM.

devsnek · 2018-03-04T21:30:45Z

cc @nodejs/post-mortem

bnoordhuis · 2018-03-05T06:22:10Z

src/node_context_data.h

+#endif
+
+enum ContextEmbedderIndex {
+  kEmbedderData = NODE_CONTEXT_EMBEDDER_DATA_INDEX,


Perhaps kEnvironment is a better name now.

Fishrock123

CI: https://ci.nodejs.org/job/node-test-pull-request/13515/

Can't comment on the postmortem parts either, but cc @joyeecheung as a heads up to the llnode folks.

joyeecheung · 2018-03-06T11:19:51Z

Also @mmarchini

mmarchini

LGTM

jasnell · 2018-03-06T16:47:52Z

Unspecified failure on one of the linux cases, from the failure it's not clear if it's unrelated although looking at the test it should be. Running again just to be safe: https://ci.nodejs.org/job/node-test-commit-linux/16891/

TimothyGu

LGTM. I'd rename the node_context_data.h file to just context_data.h.

TimothyGu · 2018-03-06T18:27:50Z

src/node_context_data.h

+// Context::SetAlignedPointerInEmbedderData() is backed by a FixedArray,
+// worst case we pay a one-time penalty for resizing the array.
+#ifndef NODE_CONTEXT_EMBEDDER_DATA_INDEX
+#define NODE_CONTEXT_EMBEDDER_DATA_INDEX 32


Could rename this EMBEDDER_DATA to ENVIRONMENT as well?

i suspect renaming this would require a docs deprecation for all of v10 and then landing this pr on v11?

This macro used to be in env.h, which is not a public header.

@devsnek raised the argument of Node.js embedding over IRC. I'm fine with removing this macro after a major bump then.

devsnek · 2018-03-07T18:20:07Z

landed in c9b4de5

PR-URL: #19135 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matheus Marchini <matheus@sthima.com>

targos · 2018-03-17T13:38:37Z

Is it OK to land this on v9.x? I'm not sure if it changes the public API

MylesBorins · 2018-03-20T11:06:00Z

when this lands on v9.x it causes compilation to break 😢

/src/node_file.o ../src/node_file.cc
../src/node_contextify.cc:225:41: warning: 'GetDebugContext' is deprecated [-Wdeprecated-declarations]
  Local<Context> debug_context = Debug::GetDebugContext(args.GetIsolate());
                                        ^
../deps/v8/include/v8-debug.h:208:3: note: 'GetDebugContext' has been explicitly marked deprecated here
  V8_DEPRECATED("Use v8-inspector",
  ^
../deps/v8/include/v8config.h:321:29: note: expanded from macro 'V8_DEPRECATED'
  declarator __attribute__((deprecated))
                            ^
../src/node_contextify.cc:230:12: warning: 'SetDebugEventListener' is deprecated [-Wdeprecated-declarations]
    Debug::SetDebugEventListener(args.GetIsolate(), dummy_event_listener);
           ^
../deps/v8/include/v8-debug.h:145:3: note: 'SetDebugEventListener' has been explicitly marked deprecated here
  V8_DEPRECATED("No longer supported", static bool SetDebugEventListener(
  ^
../deps/v8/include/v8config.h:321:29: note: expanded from macro 'V8_DEPRECATED'
  declarator __attribute__((deprecated))
                            ^
../src/node_contextify.cc:231:28: warning: 'GetDebugContext' is deprecated [-Wdeprecated-declarations]
    debug_context = Debug::GetDebugContext(args.GetIsolate());
                           ^
../deps/v8/include/v8-debug.h:208:3: note: 'GetDebugContext' has been explicitly marked deprecated here
  V8_DEPRECATED("Use v8-inspector",
  ^
../deps/v8/include/v8config.h:321:29: note: expanded from macro 'V8_DEPRECATED'
  declarator __attribute__((deprecated))
                            ^
../src/node_contextify.cc:239:23: error: no member named 'kContextEmbedderDataIndex' in 'node::Environment'; did you mean 'v8::internal::Internals::kContextEmbedderDataIndex'?
    const int index = Environment::kContextEmbedderDataIndex;
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                      v8::internal::Internals::kContextEmbedderDataIndex
../deps/v8/include/v8.h:9044:20: note: 'v8::internal::Internals::kContextEmbedderDataIndex' declared here
  static const int kContextEmbedderDataIndex = 5;
                   ^
3 warnings and 1 error generated.
make[1]: *** [/Users/mborins/code/node/v9.x/out/Release/obj.target/node_lib/src/node_contextify.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [node] Error 2

PR-URL: nodejs#19135 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matheus Marchini <matheus@sthima.com>

jasnell · 2018-08-17T23:21:08Z

Should this be backported to 8.x? If so, a separate backport PR is needed.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 4, 2018

TimothyGu approved these changes Mar 4, 2018

View reviewed changes

bnoordhuis approved these changes Mar 5, 2018

View reviewed changes

src: standardise context embedder indices

fb1844c

devsnek force-pushed the refactor/context-embedder-data-indices branch from 6ec91af to fb1844c Compare March 5, 2018 15:08

Fishrock123 approved these changes Mar 5, 2018

View reviewed changes

cjihrig approved these changes Mar 5, 2018

View reviewed changes

jasnell approved these changes Mar 6, 2018

View reviewed changes

joyeecheung approved these changes Mar 6, 2018

View reviewed changes

mmarchini approved these changes Mar 6, 2018

View reviewed changes

devsnek added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 6, 2018

TimothyGu approved these changes Mar 6, 2018

View reviewed changes

devsnek closed this Mar 7, 2018

devsnek deleted the refactor/context-embedder-data-indices branch March 7, 2018 18:20

targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 17, 2018

MylesBorins added the backport-requested-v9.x label Mar 20, 2018

jasnell added the backport-requested-v8.x label Aug 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: standardise context embedder indices #19135

src: standardise context embedder indices #19135

devsnek commented Mar 4, 2018 •

edited

Loading

TimothyGu left a comment

devsnek commented Mar 4, 2018

bnoordhuis Mar 5, 2018

Fishrock123 left a comment

joyeecheung commented Mar 6, 2018

mmarchini left a comment

jasnell commented Mar 6, 2018

TimothyGu left a comment

TimothyGu Mar 6, 2018

devsnek Mar 6, 2018 •

edited

Loading

TimothyGu Mar 7, 2018

TimothyGu Mar 7, 2018

devsnek commented Mar 7, 2018

targos commented Mar 17, 2018

MylesBorins commented Mar 20, 2018

jasnell commented Aug 17, 2018

src: standardise context embedder indices #19135

src: standardise context embedder indices #19135

Conversation

devsnek commented Mar 4, 2018 • edited Loading

Checklist

Affected core subsystem(s)

TimothyGu left a comment

Choose a reason for hiding this comment

devsnek commented Mar 4, 2018

bnoordhuis Mar 5, 2018

Choose a reason for hiding this comment

Fishrock123 left a comment

Choose a reason for hiding this comment

joyeecheung commented Mar 6, 2018

mmarchini left a comment

Choose a reason for hiding this comment

jasnell commented Mar 6, 2018

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu Mar 6, 2018

Choose a reason for hiding this comment

devsnek Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

TimothyGu Mar 7, 2018

Choose a reason for hiding this comment

TimothyGu Mar 7, 2018

Choose a reason for hiding this comment

devsnek commented Mar 7, 2018

targos commented Mar 17, 2018

MylesBorins commented Mar 20, 2018

jasnell commented Aug 17, 2018

devsnek commented Mar 4, 2018 •

edited

Loading

devsnek Mar 6, 2018 •

edited

Loading