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

Jump to bottom

src: split out per-binding state from Environment #32538

Closed
wants to merge 15 commits into from
Closed

src: split out per-binding state from Environment #32538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
4fba668
src: make creating per-binding data structures easier
addaleax Mar 5, 2019
824f84e
src: move HTTP/2 state out of Environment
addaleax Mar 5, 2019
9f0f5f9
src: move v8 stats buffers out of Environment
addaleax Mar 5, 2019
6da22d8
src: move http parser state out of Environment
addaleax Mar 28, 2020
ad65335
src: move fs state out of Environment
addaleax Mar 5, 2019
610dfe5
src,doc: add documentation for per-binding state pattern
addaleax Mar 28, 2020
9222827
fixup! src,doc: add documentation for per-binding state pattern
addaleax Mar 29, 2020
a68f057
fixup! src: make creating per-binding data structures easier
addaleax Mar 29, 2020
d9e7d38
fixup! src: move fs state out of Environment
addaleax Mar 29, 2020
5a2317d
fixup! src: move v8 stats buffers out of Environment
addaleax Mar 29, 2020
38e471c
fixup! src: move HTTP/2 state out of Environment
addaleax Mar 29, 2020
0273dbf
fixup! src: move http parser state out of Environment
addaleax Mar 29, 2020
72d24ee
fixup! src: move v8 stats buffers out of Environment
addaleax Mar 30, 2020
54324b6
fixup! fixup! src: move http parser state out of Environment
addaleax Mar 30, 2020
330e24b
fixup! src: move fs state out of Environment
addaleax Mar 30, 2020
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
44 changes: 44 additions & 0 deletions src/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,50 @@ void Initialize(Local<Object> target,
NODE_MODULE_CONTEXT_AWARE_INTERNAL(cares_wrap, Initialize)
```

<a id="per-binding-state">
#### Per-binding state

Some internal bindings, such as the HTTP parser, maintain internal state that
only affects that particular binding. In that case, one common way to store
that state is through the use of `Environment::BindingScope`, which gives all
new functions created within it access to an object for storing such state.
That object is always a [`BaseObject`][].

```c++
// In the HTTP parser source code file:
class BindingData : public BaseObject {
public:
BindingData(Environment* env, Local<Object> obj) : BaseObject(env, obj) {}

std::vector<char> parser_buffer;
bool parser_buffer_in_use = false;

// ...
};

// Available for binding functions, e.g. the HTTP Parser constructor:
static void New(const FunctionCallbackInfo<Value>& args) {
BindingData* binding_data = Unwrap<BindingData>(args.Data());
new Parser(binding_data, args.This());
}

// ... because the initialization function told the Environment to use this
// BindingData class for all functions created by it:
void InitializeHttpParser(Local<Object> target,
Local<Value> unused,
Local<Context> context,
void* priv) {
Environment* env = Environment::GetCurrent(context);
Environment::BindingScope<BindingData> binding_scope(env);
if (!binding_scope) return;
BindingData* binding_data = binding_scope.data;

// Created within the Environment::BindingScope
Local<FunctionTemplate> t = env->NewFunctionTemplate(Parser::New);
...
}
```

<a id="exception-handling"></a>
### Exception handling

Expand Down
7 changes: 4 additions & 3 deletions src/base_object-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,15 +100,16 @@ Environment* BaseObject::env() const {
return env_;
}

BaseObject* BaseObject::FromJSObject(v8::Local<v8::Object> obj) {
CHECK_GT(obj->InternalFieldCount(), 0);
BaseObject* BaseObject::FromJSObject(v8::Local<v8::Value> value) {
v8::Local<v8::Object> obj = value.As<v8::Object>();
DCHECK_GE(obj->InternalFieldCount(), BaseObject::kSlot);
return static_cast<BaseObject*>(
obj->GetAlignedPointerFromInternalField(BaseObject::kSlot));
}


template <typename T>
T* BaseObject::FromJSObject(v8::Local<v8::Object> object) {
T* BaseObject::FromJSObject(v8::Local<v8::Value> object) {
return static_cast<T*>(FromJSObject(object));
}

Expand Down
6 changes: 3 additions & 3 deletions src/base_object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ class BaseObject : public MemoryRetainer {
// was also passed to the `BaseObject()` constructor initially.
// This may return `nullptr` if the C++ object has not been constructed yet,
// e.g. when the JS object used `MakeLazilyInitializedJSTemplate`.
static inline BaseObject* FromJSObject(v8::Local<v8::Object> object);
static inline BaseObject* FromJSObject(v8::Local<v8::Value> object);
template <typename T>
static inline T* FromJSObject(v8::Local<v8::Object> object);
static inline T* FromJSObject(v8::Local<v8::Value> object);

// Make the `v8::Global` a weak reference and, `delete` this object once
// the JS object has been garbage collected and there are no (strong)
Expand Down Expand Up @@ -152,7 +152,7 @@ class BaseObject : public MemoryRetainer {

// Global alias for FromJSObject() to avoid churn.
template <typename T>
inline T* Unwrap(v8::Local<v8::Object> obj) {
inline T* Unwrap(v8::Local<v8::Value> obj) {
return BaseObject::FromJSObject<T>(obj);
}

Expand Down
121 changes: 42 additions & 79 deletions src/env-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,16 +327,48 @@ inline Environment* Environment::GetCurrent(
return GetFromCallbackData(info.Data());
}

inline Environment* Environment::GetFromCallbackData(v8::Local<v8::Value> val) {
Environment* Environment::GetFromCallbackData(v8::Local<v8::Value> val) {
DCHECK(val->IsObject());
v8::Local<v8::Object> obj = val.As<v8::Object>();
DCHECK_GE(obj->InternalFieldCount(), 1);
Environment* env =
static_cast<Environment*>(obj->GetAlignedPointerFromInternalField(0));
DCHECK_GE(obj->InternalFieldCount(),
BaseObject::kInternalFieldCount);
Environment* env = Unwrap<BaseObject>(obj)->env();
DCHECK(env->as_callback_data_template()->HasInstance(obj));
return env;
}

template <typename T>
Environment::BindingScope<T>::BindingScope(Environment* env) : env(env) {
v8::Local<v8::Object> callback_data;
if (!env->MakeBindingCallbackData<T>().ToLocal(&callback_data))
return;
data = Unwrap<T>(callback_data);

// No nesting allowed currently.
CHECK_EQ(env->current_callback_data(), env->as_callback_data());
env->set_current_callback_data(callback_data);
}

template <typename T>
Environment::BindingScope<T>::~BindingScope() {
env->set_current_callback_data(env->as_callback_data());
}

template <typename T>
v8::MaybeLocal<v8::Object> Environment::MakeBindingCallbackData() {
v8::Local<v8::Function> ctor;
v8::Local<v8::Object> obj;
if (!as_callback_data_template()->GetFunction(context()).ToLocal(&ctor) ||
!ctor->NewInstance(context()).ToLocal(&obj)) {
Copy link
Member

@joyeecheung joyeecheung Apr 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for noticing this late - but I came across this change after a recent rebase, and I think this constraint about BaseObject is incorrect (see #26382 (comment)). Context-independent templates should not point to context-dependent objects, but now every templates created by Environment::FunctionTemplate, which are supposed to be context-independent, point to different callback data that are context-dependent v8::Objects that point to BaseObjects, making the templates non-snapshottable (and theoratically just incorrect, but this is only validated by V8's serializer and not at runtime). This was already violated by #26382 and this PR just splits one context-dependent callback data into multiple context-dependent ones, and force them to be that way since BaseObjects are...associated with v8::Object (which have to be context-dependent due to references to the constructors on the prototype chain).

Copy link
Member

@joyeecheung joyeecheung Apr 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Note that even before #26382, when we were using v8::External, it was still somewhat limited - by obtaining the data directly through whatever passed into FunctiontTemplate::New, we'll always end up with the same set of callback data in the function template callback even if we switch the invoking context. This PR only makes the binding data variable by context, but it ties the FunctionTemplates to the set of the data created by the main context. If the intention is to make the binding data variable by the invoking context, (for example, so that different vm contexts have different set of builtins), we need to associate the binding data with the contexts and obtain them from there (e.g. through embedder slots, or private properties on the global object, something like GetBindingData<T>(args.GetIsolate()->GetCurrentContext())), instead of associating them with and and obtaining them from the function template callback data (Unwrap<T>(args.Data()) currently being done here), since there's always only one set of data per function template.

Copy link
Member Author

@addaleax addaleax Apr 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@joyeecheung Yeah, I know about these issues, but this PR doesn’t really seem to make anything worse?

There is a commit in #32761 that would switch this back to using Externals for snapshotting, if we don’t end up with a better solution.

If the intention is to make the binding data variable by the invoking context, (for example, so that different vm contexts have different set of builtins), we need to associate the binding data with the contexts and obtain them from there (e.g. through embedder slots, or private properties on the global object, something like GetBindingData<T>(args.GetIsolate()->GetCurrentContext())), instead of associating them with and and obtaining them from the function template callback data (Unwrap<T>(args.Data()) currently being done here), since there's always only one set of data per function template.

Err, yeah, we could do that, but if you look it that way we’re just using FunctionTemplates and ObjectTemplates in a wrong way to begin with. It would be nicer if we could just create context-dependent templates, I would say.

Copy link
Member Author

Choose a reason for hiding this comment

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

… or, even better, if we could provide data to the per-context instantiations of the templates, separately.

Copy link
Member

@joyeecheung joyeecheung Apr 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I know about these issues, but this PR doesn’t really seem to make anything worse?

I think this makes it worse in that it now forces the FunctionTemplates to be associated with context-dependent BaseObjects with the BindingData machinery. In addition BaseObject implicitly implies that the object is tied to the main context. If we were to add an indirection, we could add an indirection based on v8::External directly, but an indirection based on BaseObject(which in turn relies on Object) still doesn't make too much sense to me.

It would be nicer if we could just create context-dependent templates, I would say.

From previous discussions I think that was discouraged and it is preferred to obtain the properties from contexts. FunctionTemplates and ObjectTemplates essentially belong to the isolate while the Function and Object instantiated from them belong to the context.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I know about these issues, but this PR doesn’t really seem to make anything worse?

I think this makes it worse in that it now forces the FunctionTemplates to be associated with context-dependent BaseObjects with the BindingData machinery.

… but previously it was also associated with a context-dependent object?

In addition BaseObject implicitly implies that the object is tied to the main context

In what way? We can and do create BaseObjects in core that are associated with Contexts coming from vm.createContext().

If we were to add an indirection, we could add an indirection based on v8::External directly, but an indirection based on BaseObject(which in turn relies on Object) still doesn't make too much sense to me.

I’m not sure what you have in mind, but as said above, #32761 does contain a commit that would switch this back to v8::External for snapshotting.

It would be nicer if we could just create context-dependent templates, I would say.

From previous discussions I think that was discouraged and it is preferred to obtain the properties from contexts.

Yeah, hence the addition that it would be even better for the data associated with the template to be set or filled per-context.

FunctionTemplates and ObjectTemplates essentially belong to the isolate while the Function and Object instantiated from them belong to the context.

Yeah, I know. I don’t think we’re disagreeing here.

return v8::MaybeLocal<v8::Object>();
}
T* data = new T(this, obj);
// This won't compile if T is not a BaseObject subclass.
CHECK_EQ(data, static_cast<BaseObject*>(data));
data->MakeWeak();
return obj;
}

inline Environment* Environment::GetThreadLocalEnv() {
return static_cast<Environment*>(uv_key_get(&thread_local_env));
}
Expand Down Expand Up @@ -548,79 +580,6 @@ inline double Environment::get_default_trigger_async_id() {
return default_trigger_async_id;
}

inline double* Environment::heap_statistics_buffer() const {
CHECK_NOT_NULL(heap_statistics_buffer_);
return static_cast<double*>(heap_statistics_buffer_->Data());
}

inline void Environment::set_heap_statistics_buffer(
std::shared_ptr<v8::BackingStore> backing_store) {
CHECK(!heap_statistics_buffer_); // Should be set only once.
heap_statistics_buffer_ = std::move(backing_store);
}

inline double* Environment::heap_space_statistics_buffer() const {
CHECK(heap_space_statistics_buffer_);
return static_cast<double*>(heap_space_statistics_buffer_->Data());
}

inline void Environment::set_heap_space_statistics_buffer(
std::shared_ptr<v8::BackingStore> backing_store) {
CHECK(!heap_space_statistics_buffer_); // Should be set only once.
heap_space_statistics_buffer_ = std::move(backing_store);
}

inline double* Environment::heap_code_statistics_buffer() const {
CHECK(heap_code_statistics_buffer_);
return static_cast<double*>(heap_code_statistics_buffer_->Data());
}

inline void Environment::set_heap_code_statistics_buffer(
std::shared_ptr<v8::BackingStore> backing_store) {
CHECK(!heap_code_statistics_buffer_); // Should be set only once.
heap_code_statistics_buffer_ = std::move(backing_store);
}

inline char* Environment::http_parser_buffer() const {
return http_parser_buffer_;
}

inline void Environment::set_http_parser_buffer(char* buffer) {
CHECK_NULL(http_parser_buffer_); // Should be set only once.
http_parser_buffer_ = buffer;
}

inline bool Environment::http_parser_buffer_in_use() const {
return http_parser_buffer_in_use_;
}

inline void Environment::set_http_parser_buffer_in_use(bool in_use) {
http_parser_buffer_in_use_ = in_use;
}

inline http2::Http2State* Environment::http2_state() const {
return http2_state_.get();
}

inline void Environment::set_http2_state(
std::unique_ptr<http2::Http2State> buffer) {
CHECK(!http2_state_); // Should be set only once.
http2_state_ = std::move(buffer);
}

inline AliasedFloat64Array* Environment::fs_stats_field_array() {
return &fs_stats_field_array_;
}

inline AliasedBigUint64Array* Environment::fs_stats_field_bigint_array() {
return &fs_stats_field_bigint_array_;
}

inline std::vector<std::unique_ptr<fs::FileHandleReadWrap>>&
Environment::file_handle_read_wrap_freelist() {
return file_handle_read_wrap_freelist_;
}

inline std::shared_ptr<EnvironmentOptions> Environment::options() {
return options_;
}
Expand Down Expand Up @@ -1123,7 +1082,7 @@ inline v8::Local<v8::FunctionTemplate>
v8::Local<v8::Signature> signature,
v8::ConstructorBehavior behavior,
v8::SideEffectType side_effect_type) {
v8::Local<v8::Object> external = as_callback_data();
v8::Local<v8::Object> external = current_callback_data();
return v8::FunctionTemplate::New(isolate(), callback, external,
signature, 0, behavior, side_effect_type);
}
Expand Down Expand Up @@ -1261,7 +1220,7 @@ void Environment::modify_base_object_count(int64_t delta) {
}

int64_t Environment::base_object_count() const {
return base_object_count_;
return base_object_count_ - initial_base_object_count_;
}

void Environment::set_main_utf16(std::unique_ptr<v8::String::Value> str) {
Expand Down Expand Up @@ -1321,6 +1280,10 @@ void Environment::set_process_exit_handler(
}
} // namespace node

// These two files depend on each other. Including base_object-inl.h after this
// file is the easiest way to avoid issues with that circular dependency.
#include "base_object-inl.h"

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#endif // SRC_ENV_INL_H_
52 changes: 28 additions & 24 deletions src/env.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
#include "env.h"

#include "async_wrap.h"
#include "base_object-inl.h"
#include "debug_utils-inl.h"
#include "memory_tracker-inl.h"
#include "node_buffer.h"
#include "node_context_data.h"
#include "node_errors.h"
#include "node_file.h"
#include "node_internals.h"
#include "node_options-inl.h"
#include "node_process.h"
#include "node_v8_platform-inl.h"
#include "node_worker.h"
#include "req_wrap-inl.h"
#include "stream_base.h"
#include "tracing/agent.h"
#include "tracing/traced_value.h"
#include "util-inl.h"
Expand Down Expand Up @@ -258,18 +259,30 @@ void TrackingTraceStateObserver::UpdateTraceCategoryState() {
USE(cb->Call(env_->context(), Undefined(isolate), arraysize(args), args));
}

class NoBindingData : public BaseObject {
public:
NoBindingData(Environment* env, Local<Object> obj) : BaseObject(env, obj) {}

SET_NO_MEMORY_INFO()
SET_MEMORY_INFO_NAME(NoBindingData)
SET_SELF_SIZE(NoBindingData)
};

void Environment::CreateProperties() {
HandleScope handle_scope(isolate_);
Local<Context> ctx = context();
Local<FunctionTemplate> templ = FunctionTemplate::New(isolate());
templ->InstanceTemplate()->SetInternalFieldCount(1);
Local<Object> obj = templ->GetFunction(ctx)
.ToLocalChecked()
->NewInstance(ctx)
.ToLocalChecked();
obj->SetAlignedPointerInInternalField(0, this);
set_as_callback_data(obj);
set_as_callback_data_template(templ);
{
Context::Scope context_scope(ctx);
Local<FunctionTemplate> templ = FunctionTemplate::New(isolate());
templ->InstanceTemplate()->SetInternalFieldCount(
BaseObject::kInternalFieldCount);
set_as_callback_data_template(templ);

Local<Object> obj = MakeBindingCallbackData<NoBindingData>()
.ToLocalChecked();
set_as_callback_data(obj);
set_current_callback_data(obj);
}

// Store primordials setup by the per-context script in the environment.
Local<Object> per_context_bindings =
Expand Down Expand Up @@ -329,8 +342,6 @@ Environment::Environment(IsolateData* isolate_data,
flags_(flags),
thread_id_(thread_id.id == static_cast<uint64_t>(-1) ?
AllocateEnvironmentThreadId().id : thread_id.id),
fs_stats_field_array_(isolate_, kFsStatsBufferLength),
fs_stats_field_bigint_array_(isolate_, kFsStatsBufferLength),
context_(context->GetIsolate(), context) {
// We'll be creating new objects so make sure we've entered the context.
HandleScope handle_scope(isolate());
Expand Down Expand Up @@ -417,6 +428,10 @@ Environment::Environment(IsolateData* isolate_data,
// TODO(joyeecheung): deserialize when the snapshot covers the environment
// properties.
CreateProperties();

// This adjusts the return value of base_object_count() so that tests that
// check the count do not have to account for internally created BaseObjects.
initial_base_object_count_ = base_object_count();
}

Environment::~Environment() {
Expand All @@ -428,10 +443,6 @@ Environment::~Environment() {
isolate()->GetHeapProfiler()->RemoveBuildEmbedderGraphCallback(
BuildEmbedderGraph, this);

// Make sure there are no re-used libuv wrapper objects.
// CleanupHandles() should have removed all of them.
CHECK(file_handle_read_wrap_freelist_.empty());

HandleScope handle_scope(isolate());

#if HAVE_INSPECTOR
Expand All @@ -450,8 +461,6 @@ Environment::~Environment() {
tracing_controller->RemoveTraceStateObserver(trace_state_observer_.get());
}

delete[] http_parser_buffer_;

TRACE_EVENT_NESTABLE_ASYNC_END0(
TRACING_CATEGORY_NODE1(environment), "Environment", this);

Expand All @@ -467,7 +476,7 @@ Environment::~Environment() {
}
}

CHECK_EQ(base_object_count(), 0);
CHECK_EQ(base_object_count_, 0);
}

void Environment::InitializeLibuv(bool start_profiler_idle_notifier) {
Expand Down Expand Up @@ -600,8 +609,6 @@ void Environment::CleanupHandles() {
!handle_wrap_queue_.IsEmpty()) {
uv_run(event_loop(), UV_RUN_ONCE);
}

file_handle_read_wrap_freelist_.clear();
}

void Environment::StartProfilerIdleNotifier() {
Expand Down Expand Up @@ -1081,9 +1088,6 @@ void Environment::MemoryInfo(MemoryTracker* tracker) const {
tracker->TrackField("should_abort_on_uncaught_toggle",
should_abort_on_uncaught_toggle_);
tracker->TrackField("stream_base_state", stream_base_state_);
tracker->TrackField("fs_stats_field_array", fs_stats_field_array_);
tracker->TrackField("fs_stats_field_bigint_array",
fs_stats_field_bigint_array_);
tracker->TrackField("cleanup_hooks", cleanup_hooks_);
tracker->TrackField("async_hooks", async_hooks_);
tracker->TrackField("immediate_info", immediate_info_);
Expand Down
Loading