Skip to content
This repository has been archived by the owner on Jan 31, 2018. It is now read-only.

Add compressed (gzip) support to the childProcess RPC mechanism #68

Merged
merged 3 commits into from
May 21, 2015

Conversation

dfellis
Copy link

@dfellis dfellis commented May 18, 2015

Will add tests, soon. This just confirms that I didn't break anything at the moment. :)

cc @miloconway

if(this.child) this.child.send(body);
if (this.child) {
if (this.compressed) {
zlib.gzip(new Buffer(JSON.stringify(body)), function(err, compressedJSON) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think Buffer needs to be imported:

var Buffer = require('buffer').Buffer;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Buffer is part of the global object.

damocles@moya:~/uber/multitransport-jsonrpc(childproc-gzip)$ node
> Object.keys(global)
[ 'ArrayBuffer',
  'Int8Array',
  'Uint8Array',
  'Uint8ClampedArray',
  'Int16Array',
  'Uint16Array',
  'Int32Array',
  'Uint32Array',
  'Float32Array',
  'Float64Array',
  'DataView',
  'DTRACE_NET_SERVER_CONNECTION',
  'DTRACE_NET_STREAM_END',
  'DTRACE_NET_SOCKET_READ',
  'DTRACE_NET_SOCKET_WRITE',
  'DTRACE_HTTP_SERVER_REQUEST',
  'DTRACE_HTTP_SERVER_RESPONSE',
  'DTRACE_HTTP_CLIENT_REQUEST',
  'DTRACE_HTTP_CLIENT_RESPONSE',
  'global',
  'process',
  'GLOBAL',
  'root',
  'Buffer', /// <---- Right here :)
  'setTimeout',
  'setInterval',
  'clearTimeout',
  'clearInterval',
  'setImmediate',
  'clearImmediate',
  'console',
  'module',
  'require' ]

@miloconway
Copy link

oh i see

@miloconway
Copy link

One feature I would ask for is the ability to control compression on a per-message level that way users can leverage compression on critical sections. Otherwise LGTM.

@dfellis
Copy link
Author

dfellis commented May 18, 2015

@miloconway I'd basically have to reinvent HTTP to provide that sort of header. Content-Encoding: gzip

A portion of the IPC that's in plain text and then the rest is either plain text or gzipped.

Would like to avoid that if possible -- what were you thinking of using it for?

@miloconway
Copy link

@dfellis basically if I have lots of small messages I would not necessarily want to incur the cost of compression for them. Otherwise, as a user, I would have to establish two transports, one with compression configured and the other not.

@dfellis
Copy link
Author

dfellis commented May 19, 2015

Okay. What sort of API are you looking for? Presumably you don't want to also serialize every message to check it's length and then serialize it again inside of this library.

Would there be some sort of threshold on size you provide? Or do you want a mechanism to call the method in compression mode? Or set a flag on the transport that the next request is to be compressed?

Right now you guys are the primary consumer, especially for the IPC transport. :)

@miloconway
Copy link

I think a size threshold in the options would work best - it would have the smallest impact on the library's API and would be the primary metric we would use to determine whether we want to compress or not.

@miloconway
Copy link

Other than some sections of duplicate code that seems good for refactor, LGTM!

dfellis pushed a commit that referenced this pull request May 21, 2015
Add compressed (gzip) support to the childProcess RPC mechanism
@dfellis dfellis merged commit 0c190e1 into master May 21, 2015
@dfellis
Copy link
Author

dfellis commented May 21, 2015

@miloconway published as 0.9.0 :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants