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.

Sign up for GitHub

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: invoke per-isolate setters earlier #25608

Closed
wants to merge 1 commit into from
Closed

src: invoke per-isolate setters earlier #25608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 5 additions & 4 deletions src/env.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "node_internals.h"
#include "node_native_module.h"
#include "node_options-inl.h"
#include "node_perf.h"
#include "node_platform.h"
#include "node_process.h"
#include "node_worker.h"
Expand Down Expand Up @@ -236,10 +237,10 @@ Environment::Environment(IsolateData* isolate_data,
if (options_->no_force_async_hooks_checks) {
async_hooks_.no_force_checks();
}

// TODO(addaleax): the per-isolate state should not be controlled by
// a single Environment.
isolate()->SetPromiseRejectCallback(task_queue::PromiseRejectCallback);
isolate()->AddGCPrologueCallback(performance::MarkGarbageCollectionStart,
static_cast<void*>(this));
isolate()->AddGCEpilogueCallback(performance::MarkGarbageCollectionEnd,
static_cast<void*>(this));
}

Environment::~Environment() {
Expand Down
9 changes: 1 addition & 8 deletions src/module_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,7 +712,7 @@ void ModuleWrap::Resolve(const FunctionCallbackInfo<Value>& args) {
args.GetReturnValue().Set(obj.ToLocalChecked());
}

static MaybeLocal<Promise> ImportModuleDynamically(
MaybeLocal<Promise> ModuleWrap::ImportModuleDynamically(
Local<Context> context,
Local<v8::ScriptOrModule> referrer,
Local<String> specifier) {
Expand Down Expand Up @@ -784,8 +784,6 @@ void ModuleWrap::SetImportModuleDynamicallyCallback(
CHECK(args[0]->IsFunction());
Local<Function> import_callback = args[0].As<Function>();
env->set_host_import_module_dynamically_callback(import_callback);

iso->SetHostImportModuleDynamicallyCallback(ImportModuleDynamically);
}

void ModuleWrap::HostInitializeImportMetaObjectCallback(
Expand All @@ -809,15 +807,10 @@ void ModuleWrap::HostInitializeImportMetaObjectCallback(
void ModuleWrap::SetInitializeImportMetaObjectCallback(
const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Isolate* isolate = env->isolate();

CHECK_EQ(args.Length(), 1);
CHECK(args[0]->IsFunction());
Local<Function> import_meta_callback = args[0].As<Function>();
env->set_host_initialize_import_meta_object_callback(import_meta_callback);

isolate->SetHostInitializeImportMetaObjectCallback(
HostInitializeImportMetaObjectCallback);
}

void ModuleWrap::Initialize(Local<Object> target,
Expand Down
5 changes: 4 additions & 1 deletion src/module_wrap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,10 @@ class ModuleWrap : public BaseObject {
v8::Local<v8::Context> context,
v8::Local<v8::Module> module,
v8::Local<v8::Object> meta);

static v8::MaybeLocal<v8::Promise> ImportModuleDynamically(
v8::Local<v8::Context> context,
v8::Local<v8::ScriptOrModule> referrer,
v8::Local<v8::String> specifier);
void MemoryInfo(MemoryTracker* tracker) const override {
tracker->TrackField("url", url_);
tracker->TrackField("resolve_cache", resolve_cache_);
Expand Down
13 changes: 13 additions & 0 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

#include "module_wrap.h"
#include "node_binding.h"
#include "node_buffer.h"
#include "node_constants.h"
Expand Down Expand Up @@ -1380,7 +1381,19 @@ Isolate* NewIsolate(ArrayBufferAllocator* allocator, uv_loop_t* event_loop) {
isolate->SetFatalErrorHandler(OnFatalError);
isolate->SetAllowWasmCodeGenerationCallback(AllowWasmCodeGenerationCallback);
v8::CpuProfiler::UseDetailedSourcePositionsForProfiling(isolate);
isolate->SetPromiseRejectCallback(task_queue::PromiseRejectCallback);

// NOTE: the following two callbacks currently CHECKs that the isolates
// have associated environments.
isolate->SetHostInitializeImportMetaObjectCallback(
Copy link
Member Author

@joyeecheung joyeecheung Jan 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I am not entirely sure if we should do this here, or in Environment constructor, since these two callbacks do not handle the cases when the isolates' current context does not have Environments associated. Does it make sense to simply crash if the user tries to access import() or import.meta from a context without an associated environment? Or should we return an empty Maybe/do nothing in the callbacks instead? Or should we postpone this to the Environment initialization? (But then the Environments would control per-isolate states)

Copy link
Member

Choose a reason for hiding this comment

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

It’s tricky… I think for now crashing is okay (and the best we can do), and anything more would require interaction with embedders (which would be the main source for this not working).

Copy link
Member

@devsnek devsnek Jan 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

i don't think these esm ones should have been moved. they are set in response to core opting into handling them. if the handlers don't get attached, we shouldn't have these handlers enabled.

Copy link
Member

Choose a reason for hiding this comment

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

I think I agree with @devsnek. Embedders can install their own after the call to NewIsolate().

loader::ModuleWrap::HostInitializeImportMetaObjectCallback);
isolate->SetHostImportModuleDynamicallyCallback(
loader::ModuleWrap::ImportModuleDynamically);

#if defined HAVE_DTRACE || defined HAVE_ETW
Copy link
Member

Choose a reason for hiding this comment

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

Since you're here: #if defined(HAVE_DTRACE) || defined(HAVE_ETW) - the paren-less style is unidiomatic.

isolate->AddGCPrologueCallback(DTraceGCStartCallback);
isolate->AddGCEpilogueCallback(DTraceGCEndCallback);
#endif
return isolate;
}

Expand Down
18 changes: 8 additions & 10 deletions src/node_dtrace.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,15 +247,19 @@ void DTRACE_HTTP_CLIENT_RESPONSE(const FunctionCallbackInfo<Value>& args) {
NODE_HTTP_CLIENT_RESPONSE(&conn, conn.remote, conn.port, conn.fd);
}


void dtrace_gc_start(Isolate* isolate, GCType type, GCCallbackFlags flags) {
void DTraceGCStartCallback(Isolate* isolate,
GCType type,
GCCallbackFlags flags,
void* data) {
// Previous versions of this probe point only logged type and flags.
// That's why for reasons of backwards compatibility the isolate goes last.
NODE_GC_START(type, flags, isolate);
}


void dtrace_gc_done(Isolate* isolate, GCType type, GCCallbackFlags flags) {
void DTraceGCEndCallback(Isolate* isolate,
GCType type,
GCCallbackFlags flags,
void* data) {
// Previous versions of this probe point only logged type and flags.
// That's why for reasons of backwards compatibility the isolate goes last.
NODE_GC_DONE(type, flags, isolate);
Expand Down Expand Up @@ -290,11 +294,5 @@ void InitDTrace(Environment* env, Local<Object> target) {
#ifdef HAVE_ETW
init_etw();
#endif

#if defined HAVE_DTRACE || defined HAVE_ETW
env->isolate()->AddGCPrologueCallback(dtrace_gc_start);
env->isolate()->AddGCEpilogueCallback(dtrace_gc_done);
#endif
}

} // namespace node
9 changes: 9 additions & 0 deletions src/node_dtrace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,15 @@ namespace node {

void InitDTrace(Environment* env, v8::Local<v8::Object> target);

void DTraceGCStartCallback(v8::Isolate* isolate,
v8::GCType type,
v8::GCCallbackFlags flags,
void* data);
void DTraceGCEndCallback(v8::Isolate* isolate,
v8::GCType type,
v8::GCCallbackFlags flags,
void* data);

} // namespace node

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
Expand Down
10 changes: 0 additions & 10 deletions src/node_perf.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,14 +290,6 @@ void MarkGarbageCollectionEnd(Isolate* isolate,
entry);
}


inline void SetupGarbageCollectionTracking(Environment* env) {
env->isolate()->AddGCPrologueCallback(MarkGarbageCollectionStart,
static_cast<void*>(env));
env->isolate()->AddGCEpilogueCallback(MarkGarbageCollectionEnd,
static_cast<void*>(env));
}

// Gets the name of a function
inline Local<Value> GetName(Local<Function> fn) {
Local<Value> val = fn->GetDebugName();
Expand Down Expand Up @@ -440,8 +432,6 @@ void Initialize(Local<Object> target,
env->constants_string(),
constants,
attr).ToChecked();

SetupGarbageCollectionTracking(env);
Copy link
Member

Choose a reason for hiding this comment

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

I would actually prefer for this to stay here. It makes the code more local and more obvious to me, and only adds the callbacks when they are actually used.

Copy link
Member Author

Choose a reason for hiding this comment

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

@addaleax this is currently unconditionally called when the isolate goes through bootstrapping - maybe this should be delayed until the first time the user adds an gc performance observer..

Copy link
Member

Choose a reason for hiding this comment

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

@joyeecheung If that’s feasible, it sounds good to me, but I’m more worried about code organization than performance.

Also, I’m just noticing that we never remove these listeners … that’s a bug that we should fix if we can?

Copy link
Member

Choose a reason for hiding this comment

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

See #25647 – I’m happy to close it if you want to do sth in this PR :)

Copy link
Member Author

@joyeecheung joyeecheung Jan 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@addaleax Thanks, I think I'd prefer wrapping SetupGarbageCollectionTracking into a binding method and call it from JS when it's actually needed - that's probably orthorgnal to #25647

}

} // namespace performance
Expand Down
9 changes: 9 additions & 0 deletions src/node_perf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,15 @@ class GCPerformanceEntry : public PerformanceEntry {
PerformanceGCKind gckind_;
};

void MarkGarbageCollectionStart(v8::Isolate* isolate,
v8::GCType type,
v8::GCCallbackFlags flags,
void* data);
void MarkGarbageCollectionEnd(v8::Isolate* isolate,
v8::GCType type,
v8::GCCallbackFlags flags,
void* data);

} // namespace performance
} // namespace node

Expand Down