Zlib leak ? #4172

bpaquet · 2012-10-20T09:53:10Z

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 · 2012-10-20T12:49:39Z

what version of node are you using?

bpaquet · 2012-10-20T16:21:27Z

I have the same problem with 0.8.7 and 0.8.12

Le samedi 20 octobre 2012, Isaac Z. Schlueter a écrit :

what version of node are you using?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/4172#issuecomment-9630928.

bpaquet · 2012-10-28T23:21:12Z

Any idea ?

bnoordhuis · 2012-10-29T14:35:56Z

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 · 2012-10-29T15:43:31Z

On Mon, Oct 29, 2012 at 3:36 PM, Ben Noordhuis notifications@git.luolix.topwrote:

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.

Yes, but RSS is not stable. The heap is stable, but RSS climb to 1G in few
minutes.

You can take heap snapshots with node-heapdumphttps://github.com/bnoordhuis/node-heapdumpand check if the number of objects grows significantly over time, keeping
in mind that the heap grows as well and hence contains more objects. Real
object/memory leaks are usually quite noticeable though.

I do not think problem is in heap.

Regards

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/4172#issuecomment-9868953.

bnoordhuis · 2012-10-30T00:15:39Z

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 · 2012-10-30T06:58:12Z

Thx you for the patch. Do you think this 'provisional' patch will be
integrated into the trunk ? Do you need help to qualify it ?

Regards

On Tue, Oct 30, 2012 at 1:16 AM, Ben Noordhuis notifications@git.luolix.topwrote:

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.jsindex 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.ccindex f04260c..a28f985 100644--- a/src/node_zlib.cc+++ b/src/node_zlib.cc@@ -39,7 +39,8 @@ static Persistent callback_sym;
static Persistent 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 Clear(const Arguments& args) {+ HandleScope scope;+ ZCtx *ctx = ObjectWrap::Unwrap(args.This());+ ctx->Clear();+ return scope.Close(Undefined());
}

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

ZCtx *ctx = ObjectWrap::Unwrap(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 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.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/4172#issuecomment-9890839.

bnoordhuis · 2012-10-30T14:05:59Z

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 · 2013-02-05T18:56:25Z

@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.)

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.

bnoordhuis · 2013-02-05T21:29:26Z

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

spollack · 2013-02-05T21:59:08Z

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

bnoordhuis closed this as completed in 570e4be Oct 30, 2012

darrachequesne mentioned this issue Nov 15, 2015

Release memory used by PerMessageDeflate extension websockets/ws#608

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zlib leak ? #4172

Zlib leak ? #4172

bpaquet commented Oct 20, 2012

isaacs commented Oct 20, 2012

bpaquet commented Oct 20, 2012

bpaquet commented Oct 28, 2012

bnoordhuis commented Oct 29, 2012

bpaquet commented Oct 29, 2012

bnoordhuis commented Oct 30, 2012

bpaquet commented Oct 30, 2012

bnoordhuis commented Oct 30, 2012

spollack commented Feb 5, 2013

bnoordhuis commented Feb 5, 2013

spollack commented Feb 5, 2013

Zlib leak ? #4172

Zlib leak ? #4172

Comments

bpaquet commented Oct 20, 2012

isaacs commented Oct 20, 2012

bpaquet commented Oct 20, 2012

bpaquet commented Oct 28, 2012

bnoordhuis commented Oct 29, 2012

bpaquet commented Oct 29, 2012

bnoordhuis commented Oct 30, 2012

bpaquet commented Oct 30, 2012

bnoordhuis commented Oct 30, 2012

spollack commented Feb 5, 2013

bnoordhuis commented Feb 5, 2013

spollack commented Feb 5, 2013