http2: use shared memory tracking implementation

The extensive testing done on http2 makes it easier to make sure the implementation is correct (and doesn’t diverge unnecessarily). PR-URL: #126 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs · Oct 3, 2019 · b2d2ca4 · b2d2ca4
1 parent 84d86f3
commit b2d2ca4
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 96 deletions.
diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -5,6 +5,7 @@
 #include "node_buffer.h"
 #include "node_http2.h"
 #include "node_http2_state.h"
+#include "node_mem-inl.h"
 #include "node_perf.h"
 #include "node_revert.h"
 #include "util-inl.h"
@@ -498,101 +499,20 @@ Http2Session::Callbacks::~Callbacks() {
   nghttp2_session_callbacks_del(callbacks);
 }
 
-// Track memory allocated by nghttp2 using a custom allocator.
-class Http2Session::MemoryAllocatorInfo {
- public:
-  explicit MemoryAllocatorInfo(Http2Session* session)
-      : info({ session, H2Malloc, H2Free, H2Calloc, H2Realloc }) {}
-
-  static void* H2Malloc(size_t size, void* user_data) {
-    return H2Realloc(nullptr, size, user_data);
-  }
-
-  static void* H2Calloc(size_t nmemb, size_t size, void* user_data) {
-    size_t real_size = MultiplyWithOverflowCheck(nmemb, size);
-    void* mem = H2Malloc(real_size, user_data);
-    if (mem != nullptr)
-      memset(mem, 0, real_size);
-    return mem;
-  }
-
-  static void H2Free(void* ptr, void* user_data) {
-    if (ptr == nullptr) return;  // free(null); happens quite often.
-    void* result = H2Realloc(ptr, 0, user_data);
-    CHECK_NULL(result);
-  }
-
-  static void* H2Realloc(void* ptr, size_t size, void* user_data) {
-    Http2Session* session = static_cast<Http2Session*>(user_data);
-    size_t previous_size = 0;
-    char* original_ptr = nullptr;
-
-    // We prepend each allocated buffer with a size_t containing the full
-    // size of the allocation.
-    if (size > 0) size += sizeof(size_t);
-
-    if (ptr != nullptr) {
-      // We are free()ing or re-allocating.
-      original_ptr = static_cast<char*>(ptr) - sizeof(size_t);
-      previous_size = *reinterpret_cast<size_t*>(original_ptr);
-      // This means we called StopTracking() on this pointer before.
-      if (previous_size == 0) {
-        // Fall back to the standard Realloc() function.
-        char* ret = UncheckedRealloc(original_ptr, size);
-        if (ret != nullptr)
-          ret += sizeof(size_t);
-        return ret;
-      }
-    }
-    CHECK_GE(session->current_nghttp2_memory_, previous_size);
-
-    // TODO(addaleax): Add the following, and handle NGHTTP2_ERR_NOMEM properly
-    // everywhere:
-    //
-    // if (size > previous_size &&
-    //     !session->IsAvailableSessionMemory(size - previous_size)) {
-    //  return nullptr;
-    //}
-
-    char* mem = UncheckedRealloc(original_ptr, size);
-
-    if (mem != nullptr) {
-      // Adjust the memory info counter.
-      // TODO(addaleax): Avoid the double bookkeeping we do with
-      // current_nghttp2_memory_ + AdjustAmountOfExternalAllocatedMemory
-      // and provide versions of our memory allocation utilities that take an
-      // Environment*/Isolate* parameter and call the V8 method transparently.
-      const int64_t new_size = size - previous_size;
-      session->current_nghttp2_memory_ += new_size;
-      session->env()->isolate()->AdjustAmountOfExternalAllocatedMemory(
-          new_size);
-      *reinterpret_cast<size_t*>(mem) = size;
-      mem += sizeof(size_t);
-    } else if (size == 0) {
-      session->current_nghttp2_memory_ -= previous_size;
-      session->env()->isolate()->AdjustAmountOfExternalAllocatedMemory(
-          -static_cast<int64_t>(previous_size));
-    }
-
-    return mem;
-  }
-
-  static void StopTracking(Http2Session* session, void* ptr) {
-    size_t* original_ptr = reinterpret_cast<size_t*>(
-        static_cast<char*>(ptr) - sizeof(size_t));
-    session->current_nghttp2_memory_ -= *original_ptr;
-    session->env()->isolate()->AdjustAmountOfExternalAllocatedMemory(
-        -static_cast<int64_t>(*original_ptr));
-    *original_ptr = 0;
-  }
+void Http2Session::StopTrackingRcbuf(nghttp2_rcbuf* buf) {
+  StopTrackingMemory(buf);
+}
 
-  inline nghttp2_mem* operator*() { return &info; }
+void Http2Session::CheckAllocatedSize(size_t previous_size) const {
+  CHECK_GE(current_nghttp2_memory_, previous_size);
+}
 
-  nghttp2_mem info;
-};
+void Http2Session::IncreaseAllocatedSize(size_t size) {
+  current_nghttp2_memory_ += size;
+}
 
-void Http2Session::StopTrackingRcbuf(nghttp2_rcbuf* buf) {
-  MemoryAllocatorInfo::StopTracking(this, buf);
+void Http2Session::DecreaseAllocatedSize(size_t size) {
+  current_nghttp2_memory_ -= size;
 }
 
 Http2Session::Http2Session(Environment* env,
@@ -629,14 +549,14 @@ Http2Session::Http2Session(Environment* env,
       nghttp2_session_server_new3 :
       nghttp2_session_client_new3;
 
-  MemoryAllocatorInfo allocator_info(this);
+  nghttp2_mem alloc_info = MakeAllocator();
 
   // This should fail only if the system is out of memory, which
   // is going to cause lots of other problems anyway, or if any
   // of the options are out of acceptable range, which we should
   // be catching before it gets this far. Either way, crash if this
   // fails.
-  CHECK_EQ(fn(&session_, callbacks, this, *opts, *allocator_info), 0);
+  CHECK_EQ(fn(&session_, callbacks, this, *opts, &alloc_info), 0);
 
   outgoing_storage_.reserve(1024);
   outgoing_buffers_.reserve(32);

diff --git a/src/node_http2.h b/src/node_http2.h
@@ -8,6 +8,7 @@
 #include "nghttp2/nghttp2.h"
 
 #include "node_http2_state.h"
+#include "node_mem.h"
 #include "node_perf.h"
 #include "stream_base-inl.h"
 #include "string_bytes.h"
@@ -690,7 +691,9 @@ enum SessionBitfieldFlags {
   kSessionHasAltsvcListeners
 };
 
-class Http2Session : public AsyncWrap, public StreamListener {
+class Http2Session : public AsyncWrap,
+                     public StreamListener,
+                     public mem::NgLibMemoryManager<Http2Session, nghttp2_mem> {
  public:
   Http2Session(Environment* env,
                Local<Object> wrap,
@@ -699,7 +702,6 @@ class Http2Session : public AsyncWrap, public StreamListener {
 
   class Http2Ping;
   class Http2Settings;
-  class MemoryAllocatorInfo;
 
   void EmitStatistics();
 
@@ -800,6 +802,11 @@ class Http2Session : public AsyncWrap, public StreamListener {
   void OnStreamRead(ssize_t nread, const uv_buf_t& buf) override;
   void OnStreamAfterWrite(WriteWrap* w, int status) override;
 
+  // Implementation for mem::NgLibMemoryManager
+  void CheckAllocatedSize(size_t previous_size) const;
+  void IncreaseAllocatedSize(size_t size);
+  void DecreaseAllocatedSize(size_t size);
+
   // The JavaScript API
   static void New(const FunctionCallbackInfo<Value>& args);
   static void Consume(const FunctionCallbackInfo<Value>& args);