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.

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: switch from QUEUE to intrusive list #667

Merged
merged 4 commits into from
Feb 11, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -144,12 +144,12 @@
'src/node_wrap.h',
'src/node_i18n.h',
'src/pipe_wrap.h',
'src/queue.h',
'src/smalloc.h',
'src/tty_wrap.h',
'src/tcp_wrap.h',
'src/udp_wrap.h',
'src/req_wrap.h',
'src/req-wrap.h',
'src/req-wrap-inl.h',
'src/string_bytes.h',
'src/stream_wrap.h',
'src/tree.h',
Expand Down
15 changes: 9 additions & 6 deletions src/async-wrap-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@ inline AsyncWrap::AsyncWrap(Environment* env,
v8::Handle<v8::Object> object,
ProviderType provider,
AsyncWrap* parent)
: BaseObject(env, object),
has_async_queue_(false),
provider_type_(provider) {
: BaseObject(env, object), bits_(static_cast<uint32_t>(provider) << 1) {
// Check user controlled flag to see if the init callback should run.
if (!env->using_asyncwrap())
return;
Expand Down Expand Up @@ -49,7 +47,7 @@ inline AsyncWrap::AsyncWrap(Environment* env,
if (try_catch.HasCaught())
FatalError("node::AsyncWrap::AsyncWrap", "init hook threw");

has_async_queue_ = true;
bits_ |= 1; // has_async_queue() is true now.
Copy link
Member

Choose a reason for hiding this comment

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

Magic constant? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree it's not super pretty. Once I find a few more spots like this, I'll introduce a BitField class.


if (parent != nullptr) {
env->async_hooks_post_function()->Call(parent_obj, 0, nullptr);
Expand All @@ -59,8 +57,13 @@ inline AsyncWrap::AsyncWrap(Environment* env,
}


inline uint32_t AsyncWrap::provider_type() const {
return provider_type_;
inline bool AsyncWrap::has_async_queue() const {
return static_cast<bool>(bits_ & 1);
}


inline AsyncWrap::ProviderType AsyncWrap::provider_type() const {
return static_cast<ProviderType>(bits_ >> 1);
}


Expand Down
4 changes: 2 additions & 2 deletions src/async-wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ Handle<Value> AsyncWrap::MakeCallback(const Handle<Function> cb,
}
}

if (has_async_queue_) {
if (has_async_queue()) {
try_catch.SetVerbose(false);
env()->async_hooks_pre_function()->Call(context, 0, nullptr);
if (try_catch.HasCaught())
Expand All @@ -118,7 +118,7 @@ Handle<Value> AsyncWrap::MakeCallback(const Handle<Function> cb,
return Undefined(env()->isolate());
}

if (has_async_queue_) {
if (has_async_queue()) {
try_catch.SetVerbose(false);
env()->async_hooks_post_function()->Call(context, 0, nullptr);
if (try_catch.HasCaught())
Expand Down
11 changes: 7 additions & 4 deletions src/async-wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@
#define SRC_ASYNC_WRAP_H_

#include "base-object.h"
#include "env.h"
#include "v8.h"

#include <stdint.h>

namespace node {

#define NODE_ASYNC_PROVIDER_TYPES(V) \
Expand All @@ -31,6 +32,8 @@ namespace node {
V(WRITEWRAP) \
V(ZLIB)

class Environment;

class AsyncWrap : public BaseObject {
public:
enum ProviderType {
Expand All @@ -47,7 +50,7 @@ class AsyncWrap : public BaseObject {

inline virtual ~AsyncWrap() override = default;

inline uint32_t provider_type() const;
inline ProviderType provider_type() const;

// Only call these within a valid HandleScope.
v8::Handle<v8::Value> MakeCallback(const v8::Handle<v8::Function> cb,
Expand All @@ -62,12 +65,12 @@ class AsyncWrap : public BaseObject {

private:
inline AsyncWrap();
inline bool has_async_queue() const;

// When the async hooks init JS function is called from the constructor it is
// expected the context object will receive a _asyncQueue object property
// that will be used to call pre/post in MakeCallback.
bool has_async_queue_;
ProviderType provider_type_;
uint32_t bits_;
};

} // namespace node
Expand Down
2 changes: 2 additions & 0 deletions src/base-object-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
#define SRC_BASE_OBJECT_INL_H_

#include "base-object.h"
#include "env.h"
#include "env-inl.h"
#include "util.h"
#include "util-inl.h"
#include "v8.h"
Expand Down
3 changes: 2 additions & 1 deletion src/base-object.h
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
#ifndef SRC_BASE_OBJECT_H_
#define SRC_BASE_OBJECT_H_

#include "env.h"
#include "v8.h"

namespace node {

class Environment;

class BaseObject {
public:
BaseObject(Environment* env, v8::Local<v8::Object> handle);
Expand Down
3 changes: 2 additions & 1 deletion src/cares_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
#include "env.h"
#include "env-inl.h"
#include "node.h"
#include "req_wrap.h"
#include "req-wrap.h"
#include "req-wrap-inl.h"
#include "tree.h"
#include "util.h"
#include "uv.h"
Expand Down
23 changes: 6 additions & 17 deletions src/debug-agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
#include "v8-debug.h"
#include "util.h"
#include "util-inl.h"
#include "queue.h"

#include <string.h>

Expand Down Expand Up @@ -64,8 +63,6 @@ Agent::Agent(Environment* env) : state_(kNone),

err = uv_mutex_init(&message_mutex_);
CHECK_EQ(err, 0);

QUEUE_INIT(&messages_);
}


Expand All @@ -75,13 +72,8 @@ Agent::~Agent() {
uv_sem_destroy(&start_sem_);
uv_mutex_destroy(&message_mutex_);

// Clean-up messages
while (!QUEUE_EMPTY(&messages_)) {
QUEUE* q = QUEUE_HEAD(&messages_);
QUEUE_REMOVE(q);
AgentMessage* msg = ContainerOf(&AgentMessage::member, q);
while (AgentMessage* msg = messages_.PopFront())
delete msg;
}
}


Expand Down Expand Up @@ -281,13 +273,9 @@ void Agent::ChildSignalCb(uv_async_t* signal) {
Local<Object> api = PersistentToLocal(isolate, a->api_);

uv_mutex_lock(&a->message_mutex_);
while (!QUEUE_EMPTY(&a->messages_)) {
QUEUE* q = QUEUE_HEAD(&a->messages_);
AgentMessage* msg = ContainerOf(&AgentMessage::member, q);

while (AgentMessage* msg = a->messages_.PopFront()) {
// Time to close everything
if (msg->data() == nullptr) {
QUEUE_REMOVE(q);
delete msg;

MakeCallback(isolate, api, "onclose", 0, nullptr);
Expand All @@ -296,10 +284,11 @@ void Agent::ChildSignalCb(uv_async_t* signal) {

// Waiting for client, do not send anything just yet
// TODO(indutny): move this to js-land
if (a->wait_)
if (a->wait_) {
a->messages_.PushFront(msg); // Push message back into the ready queue.
break;
}

QUEUE_REMOVE(q);
Local<Value> argv[] = {
String::NewFromTwoByte(isolate,
msg->data(),
Expand All @@ -321,7 +310,7 @@ void Agent::ChildSignalCb(uv_async_t* signal) {

void Agent::EnqueueMessage(AgentMessage* message) {
uv_mutex_lock(&message_mutex_);
QUEUE_INSERT_TAIL(&messages_, &message->member);
messages_.PushBack(message);
uv_mutex_unlock(&message_mutex_);
uv_async_send(&child_signal_);
}
Expand Down
57 changes: 28 additions & 29 deletions src/debug-agent.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,11 @@
#ifndef SRC_DEBUG_AGENT_H_
#define SRC_DEBUG_AGENT_H_

#include "util.h"
#include "util-inl.h"
#include "uv.h"
#include "v8.h"
#include "v8-debug.h"
#include "queue.h"

#include <string.h>

Expand All @@ -37,7 +38,31 @@ class Environment;
namespace node {
namespace debugger {

class AgentMessage;
class AgentMessage {
public:
AgentMessage(uint16_t* val, int length) : length_(length) {
if (val == nullptr) {
data_ = val;
} else {
data_ = new uint16_t[length];
memcpy(data_, val, length * sizeof(*data_));
}
}

~AgentMessage() {
delete[] data_;
data_ = nullptr;
}

inline const uint16_t* data() const { return data_; }
inline int length() const { return length_; }

ListNode<AgentMessage> member;

private:
uint16_t* data_;
int length_;
};

class Agent {
public:
Expand Down Expand Up @@ -100,37 +125,11 @@ class Agent {
uv_loop_t child_loop_;
v8::Persistent<v8::Object> api_;

QUEUE messages_;
ListHead<AgentMessage, &AgentMessage::member> messages_;

DispatchHandler dispatch_handler_;
};

class AgentMessage {
public:
AgentMessage(uint16_t* val, int length) : length_(length) {
if (val == nullptr) {
data_ = val;
} else {
data_ = new uint16_t[length];
memcpy(data_, val, length * sizeof(*data_));
}
}

~AgentMessage() {
delete[] data_;
data_ = nullptr;
}

inline const uint16_t* data() const { return data_; }
inline int length() const { return length_; }

QUEUE member;

private:
uint16_t* data_;
int length_;
};

} // namespace debugger
} // namespace node

Expand Down
12 changes: 2 additions & 10 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -175,9 +175,6 @@ inline Environment::Environment(v8::Local<v8::Context> context,
set_binding_cache_object(v8::Object::New(isolate()));
set_module_load_list_array(v8::Array::New(isolate()));
RB_INIT(&cares_task_list_);
QUEUE_INIT(&req_wrap_queue_);
QUEUE_INIT(&handle_wrap_queue_);
QUEUE_INIT(&handle_cleanup_queue_);
handle_cleanup_waiting_ = 0;
}

Expand All @@ -193,11 +190,7 @@ inline Environment::~Environment() {
}

inline void Environment::CleanupHandles() {
while (!QUEUE_EMPTY(&handle_cleanup_queue_)) {
QUEUE* q = QUEUE_HEAD(&handle_cleanup_queue_);
QUEUE_REMOVE(q);

HandleCleanup* hc = ContainerOf(&HandleCleanup::handle_cleanup_queue_, q);
while (HandleCleanup* hc = handle_cleanup_queue_.PopFront()) {
handle_cleanup_waiting_++;
hc->cb_(this, hc->handle_, hc->arg_);
delete hc;
Expand Down Expand Up @@ -259,8 +252,7 @@ inline uv_check_t* Environment::idle_check_handle() {
inline void Environment::RegisterHandleCleanup(uv_handle_t* handle,
HandleCleanupCb cb,
void *arg) {
HandleCleanup* hc = new HandleCleanup(handle, cb, arg);
QUEUE_INSERT_TAIL(&handle_cleanup_queue_, &hc->handle_cleanup_queue_);
handle_cleanup_queue_.PushBack(new HandleCleanup(handle, cb, arg));
}

inline void Environment::FinishHandleCleanup(uv_handle_t* handle) {
Expand Down
21 changes: 13 additions & 8 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@

#include "ares.h"
#include "debug-agent.h"
#include "handle_wrap.h"
#include "req-wrap.h"
#include "tree.h"
#include "util.h"
#include "uv.h"
#include "v8.h"
#include "queue.h"

#include <stdint.h>

Expand Down Expand Up @@ -333,13 +334,12 @@ class Environment {
: handle_(handle),
cb_(cb),
arg_(arg) {
QUEUE_INIT(&handle_cleanup_queue_);
}

uv_handle_t* handle_;
HandleCleanupCb cb_;
void* arg_;
QUEUE handle_cleanup_queue_;
ListNode<HandleCleanup> handle_cleanup_queue_;
};

static inline Environment* GetCurrent(v8::Isolate* isolate);
Expand Down Expand Up @@ -453,8 +453,12 @@ class Environment {
return &debugger_agent_;
}

inline QUEUE* handle_wrap_queue() { return &handle_wrap_queue_; }
inline QUEUE* req_wrap_queue() { return &req_wrap_queue_; }
typedef ListHead<HandleWrap, &HandleWrap::handle_wrap_queue_> HandleWrapQueue;
typedef ListHead<ReqWrap<uv_req_t>, &ReqWrap<uv_req_t>::req_wrap_queue_>
ReqWrapQueue;

inline HandleWrapQueue* handle_wrap_queue() { return &handle_wrap_queue_; }
inline ReqWrapQueue* req_wrap_queue() { return &req_wrap_queue_; }

private:
static const int kIsolateSlot = NODE_ISOLATE_SLOT;
Expand Down Expand Up @@ -486,9 +490,10 @@ class Environment {
bool printed_error_;
debugger::Agent debugger_agent_;

QUEUE handle_wrap_queue_;
QUEUE req_wrap_queue_;
QUEUE handle_cleanup_queue_;
HandleWrapQueue handle_wrap_queue_;
ReqWrapQueue req_wrap_queue_;
ListHead<HandleCleanup,
&HandleCleanup::handle_cleanup_queue_> handle_cleanup_queue_;
int handle_cleanup_waiting_;

v8::Persistent<v8::External> external_;
Expand Down
Loading