Skip to content

Commit

Permalink
Revert "src: make IsolateData creation explicit"
Browse files Browse the repository at this point in the history 
This reverts commit c3cd453.

nodejs#7082 was landed too fast and did
not have sufficient review time.

That PR also broke some things (testcases will follow separately).
  • Loading branch information
@ChALkeR
ChALkeR committed Jun 1, 2016
1 parent ecdae5b commit 2b14127
Show file tree
Hide file tree
Showing 5 changed files with 113 additions and 63 deletions.
10 changes: 2 additions & 8 deletions src/debug-agent.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,15 +172,9 @@ void Agent::WorkerRun() {
Local<Context> context = Context::New(isolate);

Context::Scope context_scope(context);

// FIXME(bnoordhuis) Work around V8 bug: v8::Private::ForApi() dereferences
// a nullptr when a context hasn't been entered first. The symbol registry
// is a lazily created JS object but object instantiation does not work
// without a context.
IsolateData isolate_data(isolate, &child_loop_);

Environment* env = CreateEnvironment(
&isolate_data,
isolate,
&child_loop_,
context,
arraysize(argv),
argv,
Expand Down
54 changes: 41 additions & 13 deletions src/env-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,28 @@

namespace node {

inline IsolateData* IsolateData::Get(v8::Isolate* isolate) {
return static_cast<IsolateData*>(isolate->GetData(kIsolateSlot));
}

inline IsolateData* IsolateData::GetOrCreate(v8::Isolate* isolate,
uv_loop_t* loop) {
IsolateData* isolate_data = Get(isolate);
if (isolate_data == nullptr) {
isolate_data = new IsolateData(isolate, loop);
isolate->SetData(kIsolateSlot, isolate_data);
}
isolate_data->ref_count_ += 1;
return isolate_data;
}

inline void IsolateData::Put() {
if (--ref_count_ == 0) {
isolate()->SetData(kIsolateSlot, nullptr);
delete this;
}
}

// Create string properties as internalized one byte strings.
//
// Internalized because it makes property lookups a little faster and because
Expand All @@ -24,8 +46,9 @@ namespace node {
//
// One byte because our strings are ASCII and we can safely skip V8's UTF-8
// decoding step. It's a one-time cost, but why pay it when you don't have to?
inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop)
:
inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* loop)
: event_loop_(loop),
isolate_(isolate),
#define V(PropertyName, StringValue) \
PropertyName ## _( \
isolate, \
Expand All @@ -48,12 +71,16 @@ inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop)
sizeof(StringValue) - 1).ToLocalChecked()),
PER_ISOLATE_STRING_PROPERTIES(V)
#undef V
isolate_(isolate), event_loop_(event_loop) {}
ref_count_(0) {}

inline uv_loop_t* IsolateData::event_loop() const {
return event_loop_;
}

inline v8::Isolate* IsolateData::isolate() const {
return isolate_;
}

inline Environment::AsyncHooks::AsyncHooks() {
for (int i = 0; i < kFieldsCount; i++) fields_[i] = 0;
}
Expand Down Expand Up @@ -149,9 +176,9 @@ inline void Environment::ArrayBufferAllocatorInfo::reset_fill_flag() {
fields_[kNoZeroFill] = 0;
}

inline Environment* Environment::New(IsolateData* isolate_data,
v8::Local<v8::Context> context) {
Environment* env = new Environment(isolate_data, context);
inline Environment* Environment::New(v8::Local<v8::Context> context,
uv_loop_t* loop) {
Environment* env = new Environment(context, loop);
env->AssignToContext(context);
return env;
}
Expand Down Expand Up @@ -185,11 +212,11 @@ inline Environment* Environment::GetCurrent(
return static_cast<Environment*>(data.As<v8::External>()->Value());
}

inline Environment::Environment(IsolateData* isolate_data,
v8::Local<v8::Context> context)
inline Environment::Environment(v8::Local<v8::Context> context,
uv_loop_t* loop)
: isolate_(context->GetIsolate()),
isolate_data_(isolate_data),
timer_base_(uv_now(isolate_data->event_loop())),
isolate_data_(IsolateData::GetOrCreate(context->GetIsolate(), loop)),
timer_base_(uv_now(loop)),
using_domains_(false),
printed_error_(false),
trace_sync_io_(false),
Expand Down Expand Up @@ -226,6 +253,7 @@ inline Environment::~Environment() {
#define V(PropertyName, TypeName) PropertyName ## _.Reset();
ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)
#undef V
isolate_data()->Put();

delete[] heap_statistics_buffer_;
delete[] heap_space_statistics_buffer_;
Expand Down Expand Up @@ -513,9 +541,9 @@ inline v8::Local<v8::Object> Environment::NewInternalFieldObject() {
#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue)
#define V(TypeName, PropertyName, StringValue) \
inline \
v8::Local<TypeName> IsolateData::PropertyName(v8::Isolate* isolate) const { \
v8::Local<TypeName> IsolateData::PropertyName() const { \
/* Strings are immutable so casting away const-ness here is okay. */ \
return const_cast<IsolateData*>(this)->PropertyName ## _.Get(isolate); \
return const_cast<IsolateData*>(this)->PropertyName ## _.Get(isolate()); \
}
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP)
PER_ISOLATE_STRING_PROPERTIES(VS)
Expand All @@ -527,7 +555,7 @@ inline v8::Local<v8::Object> Environment::NewInternalFieldObject() {
#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue)
#define V(TypeName, PropertyName, StringValue) \
inline v8::Local<TypeName> Environment::PropertyName() const { \
return isolate_data()->PropertyName(isolate()); \
return isolate_data()->PropertyName(); \
}
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP)
PER_ISOLATE_STRING_PROPERTIES(VS)
Expand Down
23 changes: 16 additions & 7 deletions src/env.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,20 +304,30 @@ RB_HEAD(ares_task_list, ares_task_t);

class IsolateData {
public:
inline IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop);
static inline IsolateData* GetOrCreate(v8::Isolate* isolate, uv_loop_t* loop);
inline void Put();
inline uv_loop_t* event_loop() const;

#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue)
#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue)
#define V(TypeName, PropertyName, StringValue) \
inline v8::Local<TypeName> PropertyName(v8::Isolate* isolate) const;
inline v8::Local<TypeName> PropertyName() const;
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP)
PER_ISOLATE_STRING_PROPERTIES(VS)
#undef V
#undef VS
#undef VP

private:
static const int kIsolateSlot = NODE_ISOLATE_SLOT;

inline static IsolateData* Get(v8::Isolate* isolate);
inline explicit IsolateData(v8::Isolate* isolate, uv_loop_t* loop);
inline v8::Isolate* isolate() const;

uv_loop_t* const event_loop_;
v8::Isolate* const isolate_;

#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue)
#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue)
#define V(TypeName, PropertyName, StringValue) \
Expand All @@ -328,8 +338,7 @@ class IsolateData {
#undef VS
#undef VP

v8::Isolate* const isolate_;
uv_loop_t* const event_loop_;
unsigned int ref_count_;

DISALLOW_COPY_AND_ASSIGN(IsolateData);
};
Expand Down Expand Up @@ -465,8 +474,8 @@ class Environment {
const v8::PropertyCallbackInfo<T>& info);

// See CreateEnvironment() in src/node.cc.
static inline Environment* New(IsolateData* isolate_data,
v8::Local<v8::Context> context);
static inline Environment* New(v8::Local<v8::Context> context,
uv_loop_t* loop);
inline void CleanupHandles();
inline void Dispose();

Expand Down Expand Up @@ -600,7 +609,7 @@ class Environment {
static const int kContextEmbedderDataIndex = NODE_CONTEXT_EMBEDDER_DATA_INDEX;

private:
inline Environment(IsolateData* isolate_data, v8::Local<v8::Context> context);
inline Environment(v8::Local<v8::Context> context, uv_loop_t* loop);
inline ~Environment();
inline IsolateData* isolate_data() const;

Expand Down
69 changes: 41 additions & 28 deletions src/node.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4264,6 +4264,42 @@ int EmitExit(Environment* env) {
}


// Just a convenience method
Environment* CreateEnvironment(Isolate* isolate,
Local<Context> context,
int argc,
const char* const* argv,
int exec_argc,
const char* const* exec_argv) {
Environment* env;
Context::Scope context_scope(context);

env = CreateEnvironment(isolate,
uv_default_loop(),
context,
argc,
argv,
exec_argc,
exec_argv);

LoadEnvironment(env);

return env;
}

static Environment* CreateEnvironment(Isolate* isolate,
Local<Context> context,
NodeInstanceData* instance_data) {
return CreateEnvironment(isolate,
instance_data->event_loop(),
context,
instance_data->argc(),
instance_data->argv(),
instance_data->exec_argc(),
instance_data->exec_argv());
}


static void HandleCloseCb(uv_handle_t* handle) {
Environment* env = reinterpret_cast<Environment*>(handle->data);
env->FinishHandleCleanup(handle);
Expand All @@ -4278,27 +4314,17 @@ static void HandleCleanup(Environment* env,
}


IsolateData* CreateIsolateData(Isolate* isolate, uv_loop_t* loop) {
return new IsolateData(isolate, loop);
}


void FreeIsolateData(IsolateData* isolate_data) {
delete isolate_data;
}


Environment* CreateEnvironment(IsolateData* isolate_data,
Environment* CreateEnvironment(Isolate* isolate,
uv_loop_t* loop,
Local<Context> context,
int argc,
const char* const* argv,
int exec_argc,
const char* const* exec_argv) {
Isolate* isolate = context->GetIsolate();
HandleScope handle_scope(isolate);

Context::Scope context_scope(context);
Environment* env = Environment::New(isolate_data, context);
Environment* env = Environment::New(context, loop);

isolate->SetAutorunMicrotasks(false);

Expand Down Expand Up @@ -4386,22 +4412,9 @@ static void StartNodeInstance(void* arg) {
Isolate::Scope isolate_scope(isolate);
HandleScope handle_scope(isolate);
Local<Context> context = Context::New(isolate);
Context::Scope context_scope(context);

// FIXME(bnoordhuis) Work around V8 bug: v8::Private::ForApi() dereferences
// a nullptr when a context hasn't been entered first. The symbol registry
// is a lazily created JS object but object instantiation does not work
// without a context.
IsolateData isolate_data(isolate, instance_data->event_loop());

Environment* env = CreateEnvironment(&isolate_data,
context,
instance_data->argc(),
instance_data->argv(),
instance_data->exec_argc(),
instance_data->exec_argv());

Environment* env = CreateEnvironment(isolate, context, instance_data);
array_buffer_allocator->set_env(env);
Context::Scope context_scope(context);

isolate->SetAbortOnUncaughtExceptionCallback(
ShouldAbortOnUncaughtException);
Expand Down
20 changes: 13 additions & 7 deletions src/node.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,23 +190,29 @@ NODE_EXTERN void Init(int* argc,
int* exec_argc,
const char*** exec_argv);

class IsolateData;
class Environment;

NODE_EXTERN IsolateData* CreateIsolateData(v8::Isolate* isolate,
struct uv_loop_s* loop);
NODE_EXTERN void FreeIsolateData(IsolateData* isolate_data);

NODE_EXTERN Environment* CreateEnvironment(IsolateData* isolate_data,
NODE_EXTERN Environment* CreateEnvironment(v8::Isolate* isolate,
struct uv_loop_s* loop,
v8::Local<v8::Context> context,
int argc,
const char* const* argv,
int exec_argc,
const char* const* exec_argv);

NODE_EXTERN void LoadEnvironment(Environment* env);
NODE_EXTERN void FreeEnvironment(Environment* env);

// NOTE: Calling this is the same as calling
// CreateEnvironment() + LoadEnvironment() from above.
// `uv_default_loop()` will be passed as `loop`.
NODE_EXTERN Environment* CreateEnvironment(v8::Isolate* isolate,
v8::Local<v8::Context> context,
int argc,
const char* const* argv,
int exec_argc,
const char* const* exec_argv);


NODE_EXTERN void EmitBeforeExit(Environment* env);
NODE_EXTERN int EmitExit(Environment* env);
NODE_EXTERN void RunAtExit(Environment* env);
Expand Down

0 comments on commit 2b14127

Please sign in to comment.