stream_base: WriteWrap::New/::Dispose

Encapsulate allocation/disposal of `WriteWrap` instances into the `WriteWrap` class itself. PR-URL: #1090 Reviewed-By: Ben Noordhuis <[email protected]>
nodejs · Mar 7, 2015 · fe36076 · fe36076
1 parent 7f4c95e
commit fe36076
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 47 deletions.
diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h
@@ -15,6 +15,7 @@ using v8::FunctionTemplate;
 using v8::Handle;
 using v8::HandleScope;
 using v8::Local;
+using v8::Object;
 using v8::PropertyAttribute;
 using v8::PropertyCallbackInfo;
 using v8::String;
@@ -82,6 +83,31 @@ void StreamBase::JSMethod(const FunctionCallbackInfo<Value>& args) {
   args.GetReturnValue().Set((wrap->*Method)(args));
 }
 
+
+WriteWrap* WriteWrap::New(Environment* env,
+                          Local<Object> obj,
+                          StreamBase* wrap,
+                          DoneCb cb,
+                          size_t extra) {
+  size_t storage_size = ROUND_UP(sizeof(WriteWrap), kAlignSize) + extra;
+  char* storage = new char[storage_size];
+
+  return new(storage) WriteWrap(env, obj, wrap, cb);
+}
+
+
+void WriteWrap::Dispose() {
+  this->~WriteWrap();
+  delete[] reinterpret_cast<char*>(this);
+}
+
+
+char* WriteWrap::Extra(size_t offset) {
+  return reinterpret_cast<char*>(this) +
+         ROUND_UP(sizeof(*this), kAlignSize) +
+         offset;
+}
+
 }  // namespace node
 
 #endif  // SRC_STREAM_BASE_INL_H_
diff --git a/src/stream_base.cc b/src/stream_base.cc
@@ -1,4 +1,5 @@
 #include "stream_base.h"
+#include "stream_base-inl.h"
 #include "stream_wrap.h"
 
 #include "node.h"
@@ -108,6 +109,8 @@ int StreamBase::Writev(const FunctionCallbackInfo<Value>& args) {
   // Determine storage size first
   size_t storage_size = 0;
   for (size_t i = 0; i < count; i++) {
+    storage_size = ROUND_UP(storage_size, WriteWrap::kAlignSize);
+
     Handle<Value> chunk = chunks->Get(i * 2);
 
     if (Buffer::HasInstance(chunk))
@@ -124,7 +127,7 @@ int StreamBase::Writev(const FunctionCallbackInfo<Value>& args) {
     else
       chunk_size = StringBytes::StorageSize(env->isolate(), string, encoding);
 
-    storage_size += chunk_size + 15;
+    storage_size += chunk_size;
   }
 
   if (storage_size > INT_MAX)
@@ -133,13 +136,14 @@ int StreamBase::Writev(const FunctionCallbackInfo<Value>& args) {
   if (ARRAY_SIZE(bufs_) < count)
     bufs = new uv_buf_t[count];
 
-  storage_size += sizeof(WriteWrap);
-  char* storage = new char[storage_size];
-  WriteWrap* req_wrap =
-      new(storage) WriteWrap(env, req_wrap_obj, this, AfterWrite);
+  WriteWrap* req_wrap = WriteWrap::New(env,
+                                       req_wrap_obj,
+                                       this,
+                                       AfterWrite,
+                                       storage_size);
 
   uint32_t bytes = 0;
-  size_t offset = sizeof(WriteWrap);
+  size_t offset = 0;
   for (size_t i = 0; i < count; i++) {
     Handle<Value> chunk = chunks->Get(i * 2);
 
@@ -152,9 +156,9 @@ int StreamBase::Writev(const FunctionCallbackInfo<Value>& args) {
     }
 
     // Write string
-    offset = ROUND_UP(offset, 16);
+    offset = ROUND_UP(offset, WriteWrap::kAlignSize);
     CHECK_LT(offset, storage_size);
-    char* str_storage = storage + offset;
+    char* str_storage = req_wrap->Extra(offset);
     size_t str_size = storage_size - offset;
 
     Handle<String> string = chunk->ToString(env->isolate());
@@ -187,10 +191,8 @@ int StreamBase::Writev(const FunctionCallbackInfo<Value>& args) {
     ClearError();
   }
 
-  if (err) {
-    req_wrap->~WriteWrap();
-    delete[] storage;
-  }
+  if (err)
+    req_wrap->Dispose();
 
   return err;
 }
@@ -207,7 +209,6 @@ int StreamBase::WriteBuffer(const FunctionCallbackInfo<Value>& args) {
   const char* data = Buffer::Data(args[1]);
   size_t length = Buffer::Length(args[1]);
 
-  char* storage;
   WriteWrap* req_wrap;
   uv_buf_t buf;
   buf.base = const_cast<char*>(data);
@@ -224,17 +225,14 @@ int StreamBase::WriteBuffer(const FunctionCallbackInfo<Value>& args) {
   CHECK_EQ(count, 1);
 
   // Allocate, or write rest
-  storage = new char[sizeof(WriteWrap)];
-  req_wrap = new(storage) WriteWrap(env, req_wrap_obj, this, AfterWrite);
+  req_wrap = WriteWrap::New(env, req_wrap_obj, this, AfterWrite);
 
   err = DoWrite(req_wrap, bufs, count, nullptr);
   req_wrap->Dispatched();
   req_wrap_obj->Set(env->async(), True(env->isolate()));
 
-  if (err) {
-    req_wrap->~WriteWrap();
-    delete[] storage;
-  }
+  if (err)
+    req_wrap->Dispose();
 
  done:
   const char* msg = Error();
@@ -275,14 +273,13 @@ int StreamBase::WriteString(const FunctionCallbackInfo<Value>& args) {
     return UV_ENOBUFS;
 
   // Try writing immediately if write size isn't too big
-  char* storage;
   WriteWrap* req_wrap;
   char* data;
   char stack_storage[16384];  // 16kb
   size_t data_size;
   uv_buf_t buf;
 
-  bool try_write = storage_size + 15 <= sizeof(stack_storage) &&
+  bool try_write = storage_size <= sizeof(stack_storage) &&
                    (!IsIPCPipe() || send_handle_obj.IsEmpty());
   if (try_write) {
     data_size = StringBytes::Write(env->isolate(),
@@ -308,11 +305,9 @@ int StreamBase::WriteString(const FunctionCallbackInfo<Value>& args) {
     CHECK_EQ(count, 1);
   }
 
-  storage = new char[sizeof(WriteWrap) + storage_size + 15];
-  req_wrap = new(storage) WriteWrap(env, req_wrap_obj, this, AfterWrite);
+  req_wrap = WriteWrap::New(env, req_wrap_obj, this, AfterWrite, storage_size);
 
-  data = reinterpret_cast<char*>(ROUND_UP(
-      reinterpret_cast<uintptr_t>(storage) + sizeof(WriteWrap), 16));
+  data = req_wrap->Extra();
 
   if (try_write) {
     // Copy partial data
@@ -355,10 +350,8 @@ int StreamBase::WriteString(const FunctionCallbackInfo<Value>& args) {
   req_wrap->Dispatched();
   req_wrap->object()->Set(env->async(), True(env->isolate()));
 
-  if (err) {
-    req_wrap->~WriteWrap();
-    delete[] storage;
-  }
+  if (err)
+    req_wrap->Dispose();
 
  done:
   const char* msg = Error();
@@ -404,8 +397,7 @@ void StreamBase::AfterWrite(WriteWrap* req_wrap, int status) {
   if (req_wrap->object()->Has(env->oncomplete_string()))
     req_wrap->MakeCallback(env->oncomplete_string(), ARRAY_SIZE(argv), argv);
 
-  req_wrap->~WriteWrap();
-  delete[] reinterpret_cast<char*>(req_wrap);
+  req_wrap->Dispose();
 }
 
 

diff --git a/src/stream_base.h b/src/stream_base.h
@@ -56,6 +56,23 @@ class ShutdownWrap : public ReqWrap<uv_shutdown_t>,
 class WriteWrap: public ReqWrap<uv_write_t>,
                  public StreamReq<WriteWrap> {
  public:
+  static inline WriteWrap* New(Environment* env,
+                               v8::Local<v8::Object> obj,
+                               StreamBase* wrap,
+                               DoneCb cb,
+                               size_t extra = 0);
+  inline void Dispose();
+  inline char* Extra(size_t offset = 0);
+
+  inline StreamBase* wrap() const { return wrap_; }
+
+  static void NewWriteWrap(const v8::FunctionCallbackInfo<v8::Value>& args) {
+    CHECK(args.IsConstructCall());
+  }
+
+  static const size_t kAlignSize = 16;
+
+ protected:
   WriteWrap(Environment* env,
             v8::Local<v8::Object> obj,
             StreamBase* wrap,
@@ -66,24 +83,16 @@ class WriteWrap: public ReqWrap<uv_write_t>,
     Wrap(obj, this);
   }
 
+  void* operator new(size_t size) = delete;
   void* operator new(size_t size, char* storage) { return storage; }
 
   // This is just to keep the compiler happy. It should never be called, since
   // we don't use exceptions in node.
   void operator delete(void* ptr, char* storage) { UNREACHABLE(); }
 
-  inline StreamBase* wrap() const {
-    return wrap_;
-  }
-
-  static void NewWriteWrap(const v8::FunctionCallbackInfo<v8::Value>& args) {
-    CHECK(args.IsConstructCall());
-  }
-
  private:
   // People should not be using the non-placement new and delete operator on a
   // WriteWrap. Ensure this never happens.
-  void* operator new(size_t size) { UNREACHABLE(); }
   void operator delete(void* ptr) { UNREACHABLE(); }
 
   StreamBase* const wrap_;

diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
@@ -295,11 +295,10 @@ void TLSWrap::EncOut() {
 
   Local<Object> req_wrap_obj =
       env()->write_wrap_constructor_function()->NewInstance();
-  char* storage = new char[sizeof(WriteWrap)];
-  WriteWrap* write_req = new(storage) WriteWrap(env(),
-                                                req_wrap_obj,
-                                                this,
-                                                EncOutCb);
+  WriteWrap* write_req = WriteWrap::New(env(),
+                                        req_wrap_obj,
+                                        this,
+                                        EncOutCb);
 
   uv_buf_t buf[ARRAY_SIZE(data)];
   for (size_t i = 0; i < count; i++)
@@ -315,8 +314,7 @@ void TLSWrap::EncOut() {
 
 void TLSWrap::EncOutCb(WriteWrap* req_wrap, int status) {
   TLSWrap* wrap = req_wrap->wrap()->Cast<TLSWrap>();
-  req_wrap->~WriteWrap();
-  delete[] reinterpret_cast<char*>(req_wrap);
+  req_wrap->Dispose();
 
   // Handle error
   if (status) {