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

Limit zlib concurrency #15301

Closed
STRML opened this issue Sep 9, 2017 · 5 comments
Closed

Limit zlib concurrency #15301

STRML opened this issue Sep 9, 2017 · 5 comments
Labels
feature request Issues that request new features to be added to Node.js. zlib Issues and PRs related to the zlib subsystem.

Comments

@STRML
Copy link
Contributor

STRML commented Sep 9, 2017

  • Version: Node >= 4
  • Platform: Linux
  • Subsystem: Zlib

Related to #8871 (comment) and websockets/ws#1202.

In the benchmarks in ws#1202 on both Linux and Mac platforms we've been able to reproduce a consistent performance benefit when limiting zlib.deflate() concurrency:

$ node memleak.js 5
Running with concurrency 5
Deflate: 969.515ms
Memory: { rss: 90619904,
  heapTotal: 27262976,
  heapUsed: 14900688,
  external: 13115452 }
  
$ node memleak.js Infinity
Running with concurrency Infinity
Deflate: 4416.701ms
Memory: { rss: 2881708032,
  heapTotal: 191889408,
  heapUsed: 156487272,
  external: 492249148 }

Running with reduced concurrency not only takes less time, it allocates far less memory and avoids the fragmentation issue altogether.

Moving to a new allocator to avoid the fragmentation issue would not only be error-prone, it still would not fix the performance issue.

Is there any interest in implementing a queueing mechanism for limiting concurrency on calls to zlib to take advantage of the above benefits without pushing the burden downstream?

@addaleax addaleax added feature request Issues that request new features to be added to Node.js. zlib Issues and PRs related to the zlib subsystem. labels Sep 9, 2017
@addaleax
Copy link
Member

addaleax commented Sep 9, 2017

This doesn’t seem unreasonable to me. 👍

@STRML
Copy link
Contributor Author

STRML commented Sep 10, 2017

Would you have any tips on where to get started? It's simple enough to implement some JS-based queuing such that only so many streams are created concurrently (keep a counter, push to a backlog, intercept callbacks and pop, etc) - but is there a more elegant way anyone would suggest?

@addaleax
Copy link
Member

@STRML I don’t think this can be applied to zlib streams in general – you can have a 100 zlib streams and only a few of them processing data at any given time without running into trouble. I’d say this only applies to the one-shot async method like zlib.deflate(), zlib.gzip(), etc.

With that in mind, yes, your suggestion seems like a perfectly fine approach – Let me know if you need any help.

@STRML
Copy link
Contributor Author

STRML commented Oct 9, 2017

Unfortunately, the way that ws uses zlib is to create a Deflate and flush it per-frame. Applying this only to one-shot methods will not fix the issue.

I believe the issue needs to be farther down the stack, to where zlib itself is called, if we want to solve it for good.

@refack
Copy link
Contributor

refack commented Nov 11, 2018

@refack refack closed this as completed Nov 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants