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

refactor(NLVObject): use smart pointers for tracking object lifetime #76

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

refactor(NLVObject): use smart pointers for tracking object lifetime #76

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
6 changes: 1 addition & 5 deletions src/nlv_async_worker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ class NLVAsyncWorker : public NLVAsyncWorkerBase

private:
T handle_;

};

/**
Expand Down Expand Up @@ -304,11 +303,8 @@ class NLVLookupInstanceByValueWorker : public NLVAsyncWorkerBase
if (parentObject->IsObject()) {
childObject->Set(Nan::New("_parent").ToLocalChecked(), parentObject);
}

InstanceClass *child = Nan::ObjectWrap::Unwrap<InstanceClass>(childObject);
NLVObjectBasePtr *childPtr = new NLVObjectBasePtr(child);
child->SetParentReference(childPtr);
parent_->children_.push_back(childPtr);
child->AddToParent(parent_);
v8::Local<v8::Value> argv[] = { Nan::Null(), childObject };
callback->Call(2, argv);
}
Expand Down
103 changes: 39 additions & 64 deletions src/nlv_object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,37 +6,15 @@

#include <memory>
#include <nan.h>
#include <memory>
#include <type_traits>
#include <forward_list>

using namespace node;
using namespace v8;

class NLVObjectBase;

// hold a reference to a "child" object in a way that can be safely invalidated
// if the child is destroyed by the GC before the parent.
class NLVObjectBasePtr
{
public:
NLVObjectBasePtr(NLVObjectBase *ref) : ref_(ref), valid_(true) {}
bool IsValid() const { return valid_; }
NLVObjectBase* GetPointer() const {
if (!valid_) {
//Nan::ThrowReferenceError("attempt to access invalid NLVObjectBase pointer");
return NULL;
}

return ref_;
}

void SetInvalid() {
ref_ = NULL;
valid_ = false;
}

protected:
NLVObjectBase *ref_;
bool valid_;
};

#define NLV_STRINGIFY0(v) #v
#define NLV_STRINGIFY(v) NLV_STRINGIFY0(v)

Expand All @@ -49,38 +27,58 @@ class NLVObjectBasePtr
friend class NLVObject;


class NLVObjectBase : public Nan::ObjectWrap
class NLVObjectBase : public Nan::ObjectWrap, public std::enable_shared_from_this<NLVObjectBase>
{
public:
void AddToParent(NLVObjectBase* parent) {
parent->PushChild(shared_from_this());
}

void ClearChildren() {
for (auto& ptr: children_) {
if (auto object = ptr.lock()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mbroadst this is the reward, ability to see if those weak pointers are stil valid.

object->ClearHandle();
object->ClearChildren();
}
}
children_.clear();
}

virtual void ClearHandle() = 0;
virtual void ClearChildren() = 0;
virtual void SetParentReference(NLVObjectBasePtr *parentReference) = 0;

std::vector<NLVObjectBasePtr*> children_;
void PushChild(const std::shared_ptr<NLVObjectBase>& child) {
children_.emplace_front(child);
}
std::forward_list<std::weak_ptr<NLVObjectBase>> children_;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the case for using a std::forward_list here over a std::vector, other than you chose to emplace_front above. We're working with a collection that is not changing much, and certainly isn't going to need random access, so a std::vector should be preferred, and is likely more performant

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 think you misunderstood the use of forward_list. It's a single-way linked list which has constant addition if you push to front. And the point of it is the opposite of random access. Vector is random access and needs to be reallocated and recopied when elements are pushed to it. I'd argue forward_list better matches the use case here as children are added to the list not only at construction time but at random times.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant "random insertion" I guess, from the docs: "std::forward_list is a container that supports fast insertion and removal of elements from anywhere in the container." That's its main point in life, and because of the linked nature of it has a non-contiguous memory layout. std::vector on the other hand in terms of memory layout is contiguous and doesn't require an additional pointer-per-item overhead for linking. So to recap: std::vector is more cache-friendly, involves less allocations and doesn't have the memory overhead. At the end of the day we're really splitting hairs because this is an incredibly small dataset (if any), but I guess I just didn't see the need for a special case container here.

};

template <typename ParentClass, typename HandleType, typename CleanupHandler>
class NLVObject : public NLVObjectBase
{
std::shared_ptr<NLVObjectBase> selfPtr;
public:
typedef HandleType handle_type;

typedef typename std::remove_pointer<HandleType>::type HandleValue;

NLVObject(HandleType handle) : handle_(handle, CleanupHandler::cleanup), parentReference_(NULL) {}

NLVObject(HandleType handle) : handle_(handle, CleanupHandler::cleanup) {
}

~NLVObject() {
// calling virtual ClearHandle() will break if overridden by subclasses
ClearHandle();
if (parentReference_ != NULL) {
parentReference_->SetInvalid();
}
}

void RegisterSelf() {
selfPtr = shared_from_this();
}

static v8::Local<v8::Object> NewInstance(handle_type handle) {
Nan::EscapableHandleScope scope;
Local<Function> ctor = Nan::New<Function>(ParentClass::constructor);
Local<Object> object = Nan::NewInstance(ctor).ToLocalChecked();
ParentClass *class_instance = new ParentClass(handle);
auto shared = std::shared_ptr<ParentClass>(new ParentClass(handle), [=](ParentClass*) {
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer std::make_shared over the std::shared_ptr construction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True enough but make_shared has some limitations that make it not work in here:

  • If constructor is private or protected make_shared has no way to access it
  • make_shared does not accept a custom destructor function/lambda which is necessary to disable the default delete. Note that it it's Nan::ObjectWrap which does the deleting after v8 GC finishes. We use shared_ptr here only to get weak_ptr list of children. Sounds like abuse of the feature but it's done knowingly. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, hrm I didn't see you actually using the callback. I guess we can discuss that when we're focusing on these particular changes once you split the commits.

Copy link
Contributor

@mbroadst mbroadst Aug 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Also I think we should both try to brush up on Nan::PersistentBase, in particular its SetWeak method which allows for a finalization callback - this all sounds very close to what we're aiming for if I understand it correctly (though admittedly I spent like 2min reading through it all ;) )

In particular (from the embedder's guide): "Use a persistent handle when you need to keep a reference to an object for more than one function call, or when handle lifetimes do not correspond to C++ scopes."

// here we can now if GC has destroyed our object
});
ParentClass *class_instance = shared.get();
class_instance->RegisterSelf();
class_instance->Wrap(object);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in this case we shouldn't wrap this guy in a shared pointer. The idea of the smart pointer is to manage the lifetime of that object, assuming we have control over its lifetime. In this particular case we are about to give up ownership to v8 so it's a bit redundant here imho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this needs justification. To use make_shared_of_this there needs to be at least one shared pointer in existance to the object. While make_shared_of_this can be probably avoided, there needs to be shared_ptr creation in somewhere so that weak_ptr can be tracked.

return scope.Escape(object);
}
Expand All @@ -92,8 +90,7 @@ class NLVObject : public NLVObjectBase

const HandleType virHandle() const {
return handle_.get();
}

}

NAN_INLINE static ParentClass* Unwrap(v8::Local<v8::Object> val) {
if(!ParentClass::IsInstanceOf(val)) {
Expand Down Expand Up @@ -126,34 +123,12 @@ class NLVObject : public NLVObjectBase
return Unwrap(val)->virHandle();
}

virtual void ClearHandle() {
void ClearHandle() {
handle_.reset();
}

virtual void ClearChildren() {
std::vector<NLVObjectBasePtr*>::const_iterator it;
for (it = children_.begin(); it != children_.end(); ++it) {
NLVObjectBasePtr *ptr = *it;
if (ptr->IsValid()) {
NLVObjectBase *obj = ptr->GetPointer();
obj->ClearChildren();
obj->ClearHandle();
obj->SetParentReference(NULL);
delete ptr;
}
}

children_.clear();
}

virtual void SetParentReference(NLVObjectBasePtr *parentReference) {
parentReference_ = parentReference;
}

protected:
std::unique_ptr<HandleValue, decltype(&CleanupHandler::cleanup)> handle_;
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

NLVObjectBasePtr* parentReference_;

};

namespace NLV {
Expand Down
6 changes: 1 addition & 5 deletions src/worker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,17 +58,13 @@ namespace NLV {
Local<Object> childObject = T::NewInstance(val);
Local<Value> parentObject = worker->GetFromPersistent("parent");
T* child = T::Unwrap(childObject);
NLVObjectBasePtr* childPtr = new NLVObjectBasePtr(child);
if (parentObject->IsObject()) {
childObject->Set(Nan::New("_parent").ToLocalChecked(), parentObject);

auto parent = Nan::ObjectWrap::Unwrap<NLVObjectBase>(parentObject->ToObject());
if (parent) {
parent->children_.push_back(childPtr);
}
child->AddToParent(parent);
}

child->SetParentReference(childPtr);

if (try_catch.HasCaught()) {
v8::Local<v8::Value> argv[] = { try_catch.Exception() };
Expand Down