child_process: introduce binary protocol to optimizing performance by qile222 · Pull Request #393 · yodaos-project/ShadowNode

qile222 · 2018-10-25T18:32:30Z

No description provided.

Change-Id: Ide6744dcc7e43977d6cf63a70de2bef5da2a3df8

yorkie

Nice works, there are some suggestions to leave, please fix those @lolBig.

yorkie · 2018-10-25T18:33:12Z

src/js/buffer.js

  return offset + 4;
 };

+Buffer.prototype.writeInt32BE = function(value, offset, noAssert) {


Should add some comments like:

// buff.readUInt8(offset[,noAssert]) // [1] buff.readUInt8(offset) // [2] buff.readUInt8(offset, noAssert)

yorkie · 2018-10-25T18:33:53Z

src/js/buffer.js

         (this._builtin.readUInt8(offset + 3) << 24);
 };

+Buffer.prototype.readInt32BE = function(offset, noAssert) {


Ditto, BTW you need to update our Buffer docs, too.

yorkie · 2018-10-25T18:35:40Z

src/js/child_process.js

+var SIGNAL_NO = constants.os.signals;
+var INTERNAL_IPC_HEADER_LENGTH_SIZE = constants.INTERNAL_IPC_HEADER_LENGTH_SIZE;
+var INTERNAL_IPC_HEADER_SIZE = constants.INTERNAL_IPC_HEADER_SIZE;
+var INTERNAL_IPC_PAYLOAD_MAX_SIZE = constants.INTERNAL_IPC_PAYLOAD_MAX_SIZE;


Define all above constants only in child_process.js if those won't be used by others?

yorkie · 2018-10-25T18:41:40Z

src/js/child_process.js

+        var dataSize = channel.pendingBuffer.readInt32BE(headerOffset);
+        var typeOffset = headerOffset + INTERNAL_IPC_HEADER_LENGTH_SIZE;
+        var dataType = channel.pendingBuffer.readInt32BE(typeOffset);
+        var dataBegin = headerOffset + INTERNAL_IPC_HEADER_SIZE;


Could we have an abstracted IpcMessage class like the following:

while (bufLength - headerOffset >= INTERNAL_IPC_HEADER_SIZE) { var message = new IpcMessage(channel); // ... if (message.isObject()) { } }

yorkie · 2018-10-25T18:43:02Z

src/js/child_process.js

+          message = message.toString();
+        } else {
+          throw new Error('ERR_IPC_MESSAGE_TYPE_INVALID');
+        }


Ditto, we could add a method serialize() on IpcMessage to make it read easier :)

yorkie · 2018-10-25T18:48:54Z

src/modules/iotjs_module_pipe.c

+  write_data->data = malloc(sizeof(char) * size);
+  memcpy(write_data->data, data, size);
+  write_data->callback = callback;
+  uv_write_t* write_req = IOTJS_ALLOC(uv_write_t);


How about make iotjs_write_data to be iotjs_write_wrap like the following:

typedef struct iotjs_pipe_write_data { char* data; uv_write_t req; jerry_value_t callback; } iotjs_pipe_write_data;

So we could only need to malloc once :)

yorkie · 2018-10-25T18:49:22Z

src/modules/iotjs_module_pipe.c

  iotjs_jval_set_method(proto, "listen", PipeListen);
  // iotjs_jval_set_method(proto, "connect", PipeConnect);
  iotjs_jval_set_method(proto, "readStart", PipeReadStart);
+  iotjs_jval_set_method(proto, "write", Write);


It's better to use MAGIC_STRING :)

yorkie · 2018-10-25T18:49:49Z

test/run_pass/test_process_send.js

 var assert = require('assert');
+// json length is close to 19KB
 var data = require('./test_process_send.json');
+var dataStr = JSON.stringify(data)


The semi-colon :(

yorkie · 2018-10-25T18:49:56Z

test/run_pass/test_process_send.js

+// json length is close to 19KB
 var data = require('./test_process_send.json');
+var dataStr = JSON.stringify(data)
+var name = process.send ? 'child' : 'parent'


yorkie · 2018-10-25T18:51:47Z

test/run_pass/test_process_send.js

+  if (times <= 0) {
+    clearInterval(timer);
+  }
+}, 500)


This test is more like as a benchmark script, we actually need the unit tests, could you please add an case that just acquire the API works and move this to benchmark?

yorkie · 2018-10-25T18:53:41Z

By the way, @lolBig you forget to add your test into "testsets.json" :)

yorkie · 2018-10-25T19:29:19Z

src/iotjs_magic_strings.h

 #define IOTJS_MAGIC_STRING_WRITESYNC "writeSync"
 #define IOTJS_MAGIC_STRING_WRITEUINT8 "writeUInt8"
 #define IOTJS_MAGIC_STRING_WRITE "write"
+#define IOTJS_MAGIC_STRING_WRITE_UTF8_STRING "writeUtf8String"


IOTJS_MAGIC_STRING_WRITEUTF8STRING

yorkie · 2018-10-25T19:32:48Z

src/modules/iotjs_module_pipe.c


 static void iotjs_pipe_after_write(uv_write_t* req, int status) {
-  jerry_value_t callback = (jerry_value_t)(uintptr_t)req->data;
+  iotjs_write_wrap* write_data = (iotjs_write_wrap*)req->data;


Use the writer_wrap instead of write_data

yorkie

LGTM if CI is green.

)

* master: build: use CMAKE_INSTALL_PREFIX instead of custom install command (#391) child_process: introduce binary protocol to optimizing performance (#393) iotjs: cache parsed dump table for error.stack at runtime (#392) jerry: merge a59cf4f from upstream to optimize the function call (#381) promise: optimize the performance on .then and static resolve (#385) jerry: set prototype/constructor for external functions (#386) process: add shebang parser supports (#379) napi: fix complete callback segv/ticks on async-work (#380) jerry: pass regression-test-issue-2105 for heapdump (#382) tls, mqtts: fix connection not be actively disconnected (#375) websocket: fix the crash due to http exception (#376) process: add signal events support (#372)

chenximin added 10 commits October 24, 2018 03:10

refactor ipc

b06330e

bugfix

e911da5

Change-Id: Ide6744dcc7e43977d6cf63a70de2bef5da2a3df8

update

43ea280

update

3c49520

update

4b5bbe6

update

2533d8f

update

14d022e

update

997d54f

update

c90af03

update

90e6f48

qile222 requested review from legendecas and yorkie October 25, 2018 18:32

update

0e0f9c8

yorkie requested changes Oct 25, 2018

View reviewed changes

chenximin added 2 commits October 26, 2018 03:28

update

1518f8e

update

9a50240

yorkie requested changes Oct 25, 2018

View reviewed changes

update

119b393

yorkie approved these changes Oct 25, 2018

View reviewed changes

yorkie added the child_process label Oct 25, 2018

yorkie changed the title ~~child_process: optimize ipc~~ child_process: introduce binary protocol to optimizing performance Oct 25, 2018

chenximin added 2 commits October 26, 2018 04:24

update

daba6a6

update

c13d8be

yorkie merged commit 60ea12e into master Oct 25, 2018

yorkie deleted the optimize/child_process-ipc branch October 25, 2018 21:49

yorkie pushed a commit that referenced this pull request Oct 25, 2018

child_process: introduce binary protocol to optimizing performance (#393

cb68b86

)

qile222 mentioned this pull request Oct 25, 2018

child_process: crashed while sending data #388

Closed

legendecas mentioned this pull request Mar 26, 2019

parent can not get the message from the child when child forked with execPath of different iotjs version. #499

Closed

Conversation

qile222 commented Oct 25, 2018

Uh oh!

yorkie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yorkie commented Oct 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yorkie left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants