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: standardise context embedder indices #19135

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "uv.h"
#include "v8.h"
#include "node_perf_common.h"
#include "node_context_data.h"

#include <stddef.h>
#include <stdint.h>
Expand Down Expand Up @@ -283,7 +284,8 @@ inline void Environment::TickInfo::set_has_thrown(bool state) {

inline void Environment::AssignToContext(v8::Local<v8::Context> context,
const ContextInfo& info) {
context->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex, this);
context->SetAlignedPointerInEmbedderData(
ContextEmbedderIndex::kEnvironment, this);
#if HAVE_INSPECTOR
inspector_agent()->ContextCreated(context, info);
#endif // HAVE_INSPECTOR
Expand All @@ -295,7 +297,8 @@ inline Environment* Environment::GetCurrent(v8::Isolate* isolate) {

inline Environment* Environment::GetCurrent(v8::Local<v8::Context> context) {
return static_cast<Environment*>(
context->GetAlignedPointerFromEmbedderData(kContextEmbedderDataIndex));
context->GetAlignedPointerFromEmbedderData(
ContextEmbedderIndex::kEnvironment));
}

inline Environment* Environment::GetCurrent(
Expand Down Expand Up @@ -368,8 +371,8 @@ inline Environment::~Environment() {
inspector_agent_.reset();
#endif

context()->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex,
nullptr);
context()->SetAlignedPointerInEmbedderData(
ContextEmbedderIndex::kEnvironment, nullptr);

delete[] heap_statistics_buffer_;
delete[] heap_space_statistics_buffer_;
Expand Down
10 changes: 0 additions & 10 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,6 @@ struct PackageConfig {
};
} // namespace loader

// Pick an index that's hopefully out of the way when we're embedded inside
// another application. Performance-wise or memory-wise it doesn't matter:
// 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
#endif

// The number of items passed to push_values_to_array_function has diminishing
// returns around 8. This should be used at all call sites using said function.
#ifndef NODE_PUSH_VAL_TO_ARRAY_MAX
Expand Down Expand Up @@ -730,8 +722,6 @@ class Environment {
inline HandleWrapQueue* handle_wrap_queue() { return &handle_wrap_queue_; }
inline ReqWrapQueue* req_wrap_queue() { return &req_wrap_queue_; }

static const int kContextEmbedderDataIndex = NODE_CONTEXT_EMBEDDER_DATA_INDEX;

void AddPromiseHook(promise_hook_func fn, void* arg);
bool RemovePromiseHook(promise_hook_func fn, void* arg);
bool EmitNapiWarning();
Expand Down
25 changes: 25 additions & 0 deletions src/node_context_data.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#ifndef SRC_NODE_CONTEXT_DATA_H_
#define SRC_NODE_CONTEXT_DATA_H_

namespace node {

// Pick an index that's hopefully out of the way when we're embedded inside
// another application. Performance-wise or memory-wise it doesn't matter:
// 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
Copy link
Member

Choose a reason for hiding this comment

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

Could rename this EMBEDDER_DATA to ENVIRONMENT as well?

Copy link
Member Author

@devsnek devsnek Mar 6, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

#endif

#ifndef NODE_CONTEXT_SANDBOX_OBJECT_INDEX
#define NODE_CONTEXT_SANDBOX_OBJECT_INDEX 33
#endif

enum ContextEmbedderIndex {
kEnvironment = NODE_CONTEXT_EMBEDDER_DATA_INDEX,
kSandboxObject = NODE_CONTEXT_SANDBOX_OBJECT_INDEX,
};

} // namespace node

#endif // SRC_NODE_CONTEXT_DATA_H_
3 changes: 2 additions & 1 deletion src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "node_watchdog.h"
#include "base_object-inl.h"
#include "node_contextify.h"
#include "node_context_data.h"

namespace node {
namespace contextify {
Expand Down Expand Up @@ -173,7 +174,7 @@ Local<Context> ContextifyContext::CreateV8Context(
// embedder data field. However, we cannot hold a reference to a v8::Context
// directly in an Object, we instead hold onto the new context's global
// object instead (which then has a reference to the context).
ctx->SetEmbedderData(kSandboxObjectIndex, sandbox_obj);
ctx->SetEmbedderData(ContextEmbedderIndex::kSandboxObject, sandbox_obj);
sandbox_obj->SetPrivate(env->context(),
env->contextify_global_private_symbol(),
ctx->Global());
Expand Down
7 changes: 2 additions & 5 deletions src/node_contextify.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,13 @@
#include "node_internals.h"
#include "node_watchdog.h"
#include "base_object-inl.h"
#include "node_context_data.h"

namespace node {
namespace contextify {

class ContextifyContext {
protected:
// V8 reserves the first field in context objects for the debugger. We use the
// second field to hold a reference to the sandbox object.
enum { kSandboxObjectIndex = 1 };

Environment* const env_;
Persistent<v8::Context> context_;

Expand Down Expand Up @@ -45,7 +42,7 @@ class ContextifyContext {

inline v8::Local<v8::Object> sandbox() const {
return v8::Local<v8::Object>::Cast(
context()->GetEmbedderData(kSandboxObjectIndex));
context()->GetEmbedderData(ContextEmbedderIndex::kSandboxObject));
}

private:
Expand Down
7 changes: 4 additions & 3 deletions src/node_postmortem_metadata.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "util-inl.h"
#include "req_wrap.h"
#include "v8abbr.h"
#include "node_context_data.h"

#define NODEDBG_SYMBOL(Name) nodedbg_ ## Name

Expand Down Expand Up @@ -34,7 +35,7 @@
V(ListNode_ReqWrap, next_, uintptr_t, ListNode<ReqWrap<uv_req_t>>::next_)

extern "C" {
int nodedbg_const_Environment__kContextEmbedderDataIndex__int;
int nodedbg_const_ContextEmbedderIndex__kEnvironment__int;
uintptr_t nodedbg_offset_ExternalString__data__uintptr_t;

#define V(Class, Member, Type, Accessor) \
Expand All @@ -46,8 +47,8 @@ uintptr_t nodedbg_offset_ExternalString__data__uintptr_t;
namespace node {

int GenDebugSymbols() {
nodedbg_const_Environment__kContextEmbedderDataIndex__int =
Environment::kContextEmbedderDataIndex;
nodedbg_const_ContextEmbedderIndex__kEnvironment__int =
ContextEmbedderIndex::kEnvironment;

nodedbg_offset_ExternalString__data__uintptr_t = NODE_OFF_EXTSTR_DATA;

Expand Down
10 changes: 5 additions & 5 deletions test/cctest/test_node_postmortem_metadata.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ extern uintptr_t
extern uintptr_t
nodedbg_offset_Environment__handle_wrap_queue___Environment_HandleWrapQueue;
extern int debug_symbols_generated;
extern int nodedbg_const_Environment__kContextEmbedderDataIndex__int;
extern int nodedbg_const_ContextEmbedderIndex__kEnvironment__int;
extern uintptr_t
nodedbg_offset_Environment_HandleWrapQueue__head___ListNode_HandleWrap;
extern uintptr_t
Expand Down Expand Up @@ -56,10 +56,10 @@ class TestReqWrap : public node::ReqWrap<uv_req_t> {
node::AsyncWrap::PROVIDER_TIMERWRAP) {}
};

TEST_F(DebugSymbolsTest, ContextEmbedderDataIndex) {
int kContextEmbedderDataIndex = node::Environment::kContextEmbedderDataIndex;
EXPECT_EQ(nodedbg_const_Environment__kContextEmbedderDataIndex__int,
kContextEmbedderDataIndex);
TEST_F(DebugSymbolsTest, ContextEmbedderEnvironmentIndex) {
int kEnvironmentIndex = node::ContextEmbedderIndex::kEnvironment;
EXPECT_EQ(nodedbg_const_ContextEmbedderIndex__kEnvironment__int,
kEnvironmentIndex);
}

TEST_F(DebugSymbolsTest, ExternalStringDataOffset) {
Expand Down