Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Zlib leak ? #4172

Closed
bpaquet opened this Issue · 11 comments

4 participants

@bpaquet

When I do lot of zlib.deflate, I see

  • A constant rss usage increase
  • A very slow heap total increase
  • A cycling heap usage (gc seems to clean the heap :))

The test code is here :
https://gist.github.com/3922818

Is my usage of zlib wrong ?

@isaacs

what version of node are you using?

@bpaquet
@bpaquet

Any idea ?

@bnoordhuis

A very slow heap total increase

This is expected. When the V8 GC hits the high-water mark, it'll grow the heap (up to a limit - about 1.4/1.9 GB depending on the architecture.) If RSS is stable and the heap gets cleaned out regularly, there's probably no memory leak.

You can take heap snapshots with node-heapdump and check if the number of objects grows significantly over time. Keep in mind that the heap grows as well and hence contains more objects. Real object/memory leaks are usually quite noticeable though (disparate heap growth vs. total object count).

@bpaquet
@bnoordhuis

Okay, so what's happening in your example is technically not a memory leak. The memory is reclaimable, it's just that you're allocating it faster than the GC is reclaiming it. Here is a provisional patch

diff --git a/lib/zlib.js b/lib/zlib.js
index 7837f3f..0816089 100644
--- a/lib/zlib.js
+++ b/lib/zlib.js
@@ -150,7 +150,10 @@ function zlibBuffer(engine, buffer, callback) {
   }

   function onEnd() {
-    callback(null, Buffer.concat(buffers, nread));
+    var buf = Buffer.concat(buffers, nread);
+    buffers.splice(0, buffers.length);
+    callback(null, buf);
+    engine.clear();
   }

   engine.on('error', onError);
@@ -331,6 +334,10 @@ Zlib.prototype.write = function write(chunk, cb) {
   return empty;
 };

+Zlib.prototype.clear = function clear() {
+  return this._binding.clear();
+};
+
 Zlib.prototype.reset = function reset() {
   return this._binding.reset();
 };
diff --git a/src/node_zlib.cc b/src/node_zlib.cc
index f04260c..a28f985 100644
--- a/src/node_zlib.cc
+++ b/src/node_zlib.cc
@@ -39,7 +39,8 @@ static Persistent<String> callback_sym;
 static Persistent<String> onerror_sym;

 enum node_zlib_mode {
-  DEFLATE = 1,
+  NONE,
+  DEFLATE,
   INFLATE,
   GZIP,
   GUNZIP,
@@ -60,17 +61,40 @@ class ZCtx : public ObjectWrap {

   ZCtx(node_zlib_mode mode) : ObjectWrap(), dictionary_(NULL), mode_(mode) {}

+
   ~ZCtx() {
+    Clear();
+  }
+
+
+  void Clear() {
+    assert(!write_in_progress_ && "write in progress");
+    assert(init_done_ && "clear before init");
+    assert(mode_ <= UNZIP);
+
     if (mode_ == DEFLATE || mode_ == GZIP || mode_ == DEFLATERAW) {
       (void)deflateEnd(&strm_);
     } else if (mode_ == INFLATE || mode_ == GUNZIP || mode_ == INFLATERAW ||
                mode_ == UNZIP) {
       (void)inflateEnd(&strm_);
     }
+    mode_ = NONE;
+
+    if (dictionary_ != NULL) {
+      delete[] dictionary_;
+      dictionary_ = NULL;
+    }
+  }
+

-    if (dictionary_ != NULL) delete[] dictionary_;
+  static Handle<Value> Clear(const Arguments& args) {
+    HandleScope scope;
+    ZCtx *ctx = ObjectWrap::Unwrap<ZCtx>(args.This());
+    ctx->Clear();
+    return scope.Close(Undefined());
   }

+
   // write(flush, in, in_off, in_len, out, out_off, out_len)
   static Handle<Value> Write(const Arguments& args) {
     HandleScope scope;
@@ -78,6 +102,7 @@ class ZCtx : public ObjectWrap {

     ZCtx *ctx = ObjectWrap::Unwrap<ZCtx>(args.This());
     assert(ctx->init_done_ && "write before init");
+    assert(ctx->mode_ != NONE && "already finalized");

     assert(!ctx->write_in_progress_ && "write already in progress");
     ctx->write_in_progress_ = true;
@@ -441,6 +466,7 @@ void InitZlib(Handle<Object> target) {

   NODE_SET_PROTOTYPE_METHOD(z, "write", ZCtx::Write);
   NODE_SET_PROTOTYPE_METHOD(z, "init", ZCtx::Init);
+  NODE_SET_PROTOTYPE_METHOD(z, "clear", ZCtx::Clear);
   NODE_SET_PROTOTYPE_METHOD(z, "reset", ZCtx::Reset);

   z->SetClassName(String::NewSymbol("Zlib"));

With the above patch applied, your example tops out at a reasonable 30 MB after 10,000 iterations instead of 200+ MB. That's with a 32 bits build, a 64 bits build will have a larger memory footprint.

@bpaquet
@bnoordhuis bnoordhuis referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@bnoordhuis bnoordhuis closed this issue from a commit
@bnoordhuis bnoordhuis zlib: reduce memory consumption, release early
In zlibBuffer(), don't wait for the garbage collector to reclaim the zlib memory
but release it manually. Reduces memory consumption by a factor of 10 or more
with some workloads.

Test case:

  function f() {
    require('zlib').deflate('xxx', g);
  }
  function g() {
    setTimeout(f, 5);
  }
  f();

Observe RSS memory usage with and without this commit. After 10,000 iterations,
RSS stabilizes at ~35 MB with this commit. Without, RSS is over 300 MB and keeps
growing.

Cause: whenever the JS object heap hits the high-water mark, the V8 GC sweeps
it clean, then tries to grow it in order to avoid more sweeps in the near
future. Rule of thumb: the bigger the JS heap, the lazier the GC can be.

A side effect of a bigger heap is that objects now live longer. This is harmless
in general but it affects zlib context objects because those are tied to large
buffers that live outside the JS heap, on the order of 16K per context object.

Ergo, don't wait for the GC to reclaim the memory - it may take a long time.

Fixes #4172.
570e4be
@bnoordhuis

Do you think this 'provisional' patch will be integrated into the trunk ?

I think 'yes' - mostly because I just landed it in 570e4be and a93424d. :-)

@spollack

@bnoordhuis any chance of backporting this fix into v0.8.x?

this zlib issue is causing our RSS memory to grow to the point that it is causing our production app to thrash and fail.

(my test case, https://gist.github.com/spollack/4716706, looks very much like the one above. Under v0.8.18 i see RSS growing to huge levels, and under v0.9.8 RSS stays under control.)

@bnoordhuis bnoordhuis referenced this issue from a commit
@bnoordhuis bnoordhuis zlib: reduce memory consumption, release early
In zlibBuffer(), don't wait for the garbage collector to reclaim the zlib memory
but release it manually. Reduces memory consumption by a factor of 10 or more
with some workloads.

Test case:

  function f() {
    require('zlib').deflate('xxx', g);
  }
  function g() {
    setTimeout(f, 5);
  }
  f();

Observe RSS memory usage with and without this commit. After 10,000 iterations,
RSS stabilizes at ~35 MB with this commit. Without, RSS is over 300 MB and keeps
growing.

Cause: whenever the JS object heap hits the high-water mark, the V8 GC sweeps
it clean, then tries to grow it in order to avoid more sweeps in the near
future. Rule of thumb: the bigger the JS heap, the lazier the GC can be.

A side effect of a bigger heap is that objects now live longer. This is harmless
in general but it affects zlib context objects because those are tied to large
buffers that live outside the JS heap, on the order of 16K per context object.

Ergo, don't wait for the GC to reclaim the memory - it may take a long time.

Fixes #4172.
8d14668
@bnoordhuis

Sure. Back-ported (well, cherry-picked) in 8d14668 and 6b99fd2.

@spollack

Thanks! looking forward to v0.8.19 with this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.