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

n-api: free instance data as reference #31638

Closed
wants to merge 1 commit into from
Closed

n-api: free instance data as reference #31638

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
169 changes: 108 additions & 61 deletions src/js_native_api_v8.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,49 +186,41 @@ inline static napi_status ConcludeDeferred(napi_env env,
}

// Wrapper around v8impl::Persistent that implements reference counting.
class Reference : protected Finalizer, RefTracker {
class RefBase : protected Finalizer, RefTracker {
protected:
Reference(napi_env env,
v8::Local<v8::Value> value,
uint32_t initial_refcount,
bool delete_self,
napi_finalize finalize_callback,
void* finalize_data,
void* finalize_hint)
RefBase(napi_env env,
uint32_t initial_refcount,
bool delete_self,
napi_finalize finalize_callback,
void* finalize_data,
void* finalize_hint)
: Finalizer(env, finalize_callback, finalize_data, finalize_hint),
_persistent(env->isolate, value),
_refcount(initial_refcount),
_delete_self(delete_self) {
if (initial_refcount == 0) {
_persistent.SetWeak(
this, FinalizeCallback, v8::WeakCallbackType::kParameter);
}
Link(finalize_callback == nullptr
? &env->reflist
: &env->finalizing_reflist);
}

public:
void* Data() {
static RefBase* New(napi_env env,
uint32_t initial_refcount,
bool delete_self,
napi_finalize finalize_callback,
void* finalize_data,
void* finalize_hint) {
return new RefBase(env,
initial_refcount,
delete_self,
finalize_callback,
finalize_data,
finalize_hint);
}

inline void* Data() {
return _finalize_data;
}

static Reference* New(napi_env env,
v8::Local<v8::Value> value,
uint32_t initial_refcount,
bool delete_self,
napi_finalize finalize_callback = nullptr,
void* finalize_data = nullptr,
void* finalize_hint = nullptr) {
return new Reference(env,
value,
initial_refcount,
delete_self,
finalize_callback,
finalize_data,
finalize_hint);
}

// Delete is called in 2 ways. Either from the finalizer or
// from one of Unwrap or napi_delete_reference.
//
Expand All @@ -244,7 +236,7 @@ class Reference : protected Finalizer, RefTracker {
// The second way this is called is from
// the finalizer and _delete_self is set. In this case we
// know we need to do the deletion so just do it.
static void Delete(Reference* reference) {
static inline void Delete(RefBase* reference) {
reference->Unlink();
if ((reference->RefCount() != 0) ||
(reference->_delete_self) ||
Expand All @@ -257,40 +249,23 @@ class Reference : protected Finalizer, RefTracker {
}
}

uint32_t Ref() {
if (++_refcount == 1) {
_persistent.ClearWeak();
}

return _refcount;
inline uint32_t Ref() {
return ++_refcount;
}

uint32_t Unref() {
inline uint32_t Unref() {
if (_refcount == 0) {
return 0;
}
if (--_refcount == 0) {
_persistent.SetWeak(
this, FinalizeCallback, v8::WeakCallbackType::kParameter);
}

return _refcount;
return --_refcount;
}

uint32_t RefCount() {
inline uint32_t RefCount() {
return _refcount;
}

v8::Local<v8::Value> Get() {
if (_persistent.IsEmpty()) {
return v8::Local<v8::Value>();
} else {
return v8::Local<v8::Value>::New(_env->isolate, _persistent);
}
}

protected:
void Finalize(bool is_env_teardown = false) override {
inline void Finalize(bool is_env_teardown = false) override {
if (_finalize_callback != nullptr) {
_env->CallIntoModuleThrow([&](napi_env env) {
_finalize_callback(
Expand All @@ -310,6 +285,68 @@ class Reference : protected Finalizer, RefTracker {
}
}

private:
uint32_t _refcount;
bool _delete_self;
};

class Reference : public RefBase {
protected:
template <typename... Args>
Reference(napi_env env,
v8::Local<v8::Value> value,
Args&&... args)
: RefBase(env, std::forward<Args>(args)...),
_persistent(env->isolate, value) {
if (RefCount() == 0) {
_persistent.SetWeak(
this, FinalizeCallback, v8::WeakCallbackType::kParameter);
}
}

public:
static inline Reference* New(napi_env env,
v8::Local<v8::Value> value,
uint32_t initial_refcount,
bool delete_self,
napi_finalize finalize_callback = nullptr,
void* finalize_data = nullptr,
void* finalize_hint = nullptr) {
return new Reference(env,
value,
initial_refcount,
delete_self,
finalize_callback,
finalize_data,
finalize_hint);
}

inline uint32_t Ref() {
uint32_t refcount = RefBase::Ref();
if (refcount == 1) {
_persistent.ClearWeak();
}
return refcount;
}

inline uint32_t Unref() {
uint32_t old_refcount = RefCount();
uint32_t refcount = RefBase::Unref();
if (old_refcount == 1 && refcount == 0) {
_persistent.SetWeak(
this, FinalizeCallback, v8::WeakCallbackType::kParameter);
}
return refcount;
}

inline v8::Local<v8::Value> Get() {
if (_persistent.IsEmpty()) {
return v8::Local<v8::Value>();
} else {
return v8::Local<v8::Value>::New(_env->isolate, _persistent);
}
}

private:
// The N-API finalizer callback may make calls into the engine. V8's heap is
// not in a consistent state during the weak callback, and therefore it does
Expand All @@ -332,8 +369,6 @@ class Reference : protected Finalizer, RefTracker {
}

v8impl::Persistent<v8::Value> _persistent;
uint32_t _refcount;
bool _delete_self;
};

class ArrayBufferReference final : public Reference {
Expand All @@ -354,7 +389,7 @@ class ArrayBufferReference final : public Reference {
}

private:
void Finalize(bool is_env_teardown) override {
inline void Finalize(bool is_env_teardown) override {
if (is_env_teardown) {
v8::HandleScope handle_scope(_env->isolate);
v8::Local<v8::Value> ab = Get();
Expand Down Expand Up @@ -3111,9 +3146,19 @@ napi_status napi_set_instance_data(napi_env env,
void* finalize_hint) {
CHECK_ENV(env);

env->instance_data.data = data;
env->instance_data.finalize_cb = finalize_cb;
env->instance_data.hint = finalize_hint;
v8impl::RefBase* old_data = static_cast<v8impl::RefBase*>(env->instance_data);
if (old_data != nullptr) {
// Our contract so far has been to not finalize any old data there may be.
// So we simply delete it.
v8impl::RefBase::Delete(old_data);
}

env->instance_data = v8impl::RefBase::New(env,
0,
true,
finalize_cb,
data,
finalize_hint);

return napi_clear_last_error(env);
}
Expand All @@ -3123,7 +3168,9 @@ napi_status napi_get_instance_data(napi_env env,
CHECK_ENV(env);
CHECK_ARG(env, data);

*data = env->instance_data.data;
v8impl::RefBase* idata = static_cast<v8impl::RefBase*>(env->instance_data);

*data = (idata == nullptr ? nullptr : idata->Data());

return napi_clear_last_error(env);
}
Expand Down
54 changes: 44 additions & 10 deletions src/js_native_api_v8.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,49 @@

static napi_status napi_clear_last_error(napi_env env);

namespace v8impl {

class RefTracker {
public:
RefTracker() {}
virtual ~RefTracker() {}
virtual void Finalize(bool isEnvTeardown) {}

typedef RefTracker RefList;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can replace this entire custom linked list implementation by ListHead/ListNode from util.h? That works pretty well for other parts of the codebase :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I filed #31639 for this.


inline void Link(RefList* list) {
prev_ = list;
next_ = list->next_;
if (next_ != nullptr) {
next_->prev_ = this;
}
list->next_ = this;
}

inline void Unlink() {
if (prev_ != nullptr) {
prev_->next_ = next_;
}
if (next_ != nullptr) {
next_->prev_ = prev_;
}
prev_ = nullptr;
next_ = nullptr;
}

static void FinalizeAll(RefList* list) {
while (list->next_ != nullptr) {
list->next_->Finalize(true);
}
}

private:
RefList* next_ = nullptr;
RefList* prev_ = nullptr;
};

} // end of namespace v8impl

struct napi_env__ {
explicit napi_env__(v8::Local<v8::Context> context)
: isolate(context->GetIsolate()),
Expand All @@ -22,11 +65,6 @@ struct napi_env__ {
// `napi_finalizer` deleted them subsequently.
v8impl::RefTracker::FinalizeAll(&finalizing_reflist);
v8impl::RefTracker::FinalizeAll(&reflist);
if (instance_data.finalize_cb != nullptr) {
CallIntoModuleThrow([&](napi_env env) {
instance_data.finalize_cb(env, instance_data.data, instance_data.hint);
});
}
}
v8::Isolate* const isolate; // Shortcut for context()->GetIsolate()
v8impl::Persistent<v8::Context> context_persistent;
Expand Down Expand Up @@ -76,11 +114,7 @@ struct napi_env__ {
int open_handle_scopes = 0;
int open_callback_scopes = 0;
int refs = 1;
struct {
void* data = nullptr;
void* hint = nullptr;
napi_finalize finalize_cb = nullptr;
} instance_data;
void* instance_data = nullptr;
};

static inline napi_status napi_clear_last_error(napi_env env) {
Expand Down
39 changes: 0 additions & 39 deletions src/js_native_api_v8_internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,45 +28,6 @@

namespace v8impl {

class RefTracker {
public:
RefTracker() {}
virtual ~RefTracker() {}
virtual void Finalize(bool isEnvTeardown) {}

typedef RefTracker RefList;

inline void Link(RefList* list) {
prev_ = list;
next_ = list->next_;
if (next_ != nullptr) {
next_->prev_ = this;
}
list->next_ = this;
}

inline void Unlink() {
if (prev_ != nullptr) {
prev_->next_ = next_;
}
if (next_ != nullptr) {
next_->prev_ = prev_;
}
prev_ = nullptr;
next_ = nullptr;
}

static void FinalizeAll(RefList* list) {
while (list->next_ != nullptr) {
list->next_->Finalize(true);
}
}

private:
RefList* next_ = nullptr;
RefList* prev_ = nullptr;
};

template <typename T>
using Persistent = v8::Global<T>;

Expand Down
23 changes: 23 additions & 0 deletions test/node-api/test_instance_data/addon.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#include <stdio.h>
#include <stdlib.h>
#define NAPI_EXPERIMENTAL
#include <node_api.h>

static void addon_free(napi_env env, void* data, void* hint) {
napi_ref* ref = data;
napi_delete_reference(env, *ref);
free(ref);
fprintf(stderr, "addon_free");
}

napi_value addon_new(napi_env env, napi_value exports, bool ref_first) {
napi_ref* ref = malloc(sizeof(*ref));
if (ref_first) {
napi_create_reference(env, exports, 1, ref);
napi_set_instance_data(env, ref, addon_free, NULL);
} else {
napi_set_instance_data(env, ref, addon_free, NULL);
napi_create_reference(env, exports, 1, ref);
}
return exports;
}