Review #6

Yone-e · 2018-06-02T18:53:04Z

No description provided.

* implementing 'Writable' which has an Array of Buffers as an internal storage structure

…ich has an Buffer as an internal storage structure

SemenchenkoVitaliy

Use eslint

SemenchenkoVitaliy · 2018-08-04T11:23:29Z

lib/buffer-based/writable.js

+
+  let ret =
+    state.length < state.highWaterMark &&
+    state.bufferOffset + Buffer.from(chunk).length < state.internalBuffer.length;


make separate variable for Buffer.from(chunk) to avoid unnecessary memory allocation and copying

use state.highWaterMark instead of state.internalBuffer.length because they have the same value and in the 1st case you have less nesting

tshemsedinov · 2018-08-04T21:06:37Z

@SemenchenkoVitaliy please compare with @machendos implementation and choose better one

SemenchenkoVitaliy · 2018-08-05T07:20:00Z

If @Yone-e will fix everything that I said and add timer, i would consider it as a better one. @machendos implements stream.Writable interface only partially

tshemsedinov · 2018-08-05T10:15:21Z

Also note that CI fails: https://travis-ci.org/metarhia/metastreams/jobs/412174568

/home/travis/build/metarhia/metastreams/lib/array-based/writable.js
   19:33  error  'onwrite' was used before it was defined           no-use-before-define
   48:5   error  'writeAfterEnd' was used before it was defined     no-use-before-define
   54:12  error  'writeOrBuffer' was used before it was defined     no-use-before-define
   81:5   error  'endWritable' was used before it was defined       no-use-before-define
  105:7   error  'clearBuffer' was used before it was defined       no-use-before-define
  139:2   error  Unnecessary semicolon                              no-extra-semi
  143:22  error  'decodeChunk' was used before it was defined       no-use-before-define
  155:7   error  'ret' is never reassigned. Use 'const' instead     prefer-const
  162:1   error  Line 162 exceeds the maximum line length of 80     max-len
  179:5   error  'doWrite' was used before it was defined           no-use-before-define
  214:5   error  'onwriteError' was used before it was defined      no-use-before-define
  216:22  error  'needFinish' was used before it was defined        no-use-before-define
  226:7   error  'clearBuffer' was used before it was defined       no-use-before-define
  230:24  error  'afterWrite' was used before it was defined        no-use-before-define
  232:7   error  'afterWrite' was used before it was defined        no-use-before-define
  242:22  error  'finishMaybe' was used before it was defined       no-use-before-define
  249:5   error  'finishMaybe' was used before it was defined       no-use-before-define
  258:2   error  Unnecessary semicolon                              no-extra-semi
  262:5   error  'onwriteDrain' was used before it was defined      no-use-before-define
  267:3   error  'finishMaybe' was used before it was defined       no-use-before-define
  280:1   error  Line 280 exceeds the maximum line length of 80     max-len
  312:8   error  Unnecessary semicolon                              no-extra-semi
  338:5   error  'finishMaybe' was used before it was defined       no-use-before-define
  376:5   error  Closing curly brace does not appear on the same line
as the subsequent block brace-style
/home/travis/build/metarhia/metastreams/lib/buffer-based/writable.js
   16:1   error  Line 16 exceeds the maximum line length of 80      max-len
   20:33  error  'onwrite' was used before it was defined           no-use-before-define
   50:5   error  'writeAfterEnd' was used before it was defined     no-use-before-define
   56:12  error  'writeOrBuffer' was used before it was defined     no-use-before-define
   83:5   error  'endWritable' was used before it was defined       no-use-before-define
  105:7   error  'clearBuffer' was used before it was defined       no-use-before-define
  132:1   error  Line 132 exceeds the maximum line length of 80     max-len
  135:1   error  Line 135 exceeds the maximum line length of 80     max-len
  144:2   error  Unnecessary semicolon                              no-extra-semi
  148:22  error  'decodeChunk' was used before it was defined       no-use-before-define
  160:7   error  'ret' is never reassigned. Use 'const' instead     prefer-const
  162:1   error  Line 162 exceeds the maximum line length of 80     max-len
  171:9   error  'doWrite' was used before it was defined           no-use-before-define
  187:7   error  'doWrite' was used before it was defined           no-use-before-define
  189:1   error  Line 189 exceeds the maximum line length of 80     max-len
  190:7   error  'doWrite' was used before it was defined           no-use-before-define
  214:5   error  'doWrite' was used before it was defined           no-use-before-define
  249:5   error  'onwriteError' was used before it was defined      no-use-before-define
  251:22  error  'needFinish' was used before it was defined        no-use-before-define
  259:7   error  'clearBuffer' was used before it was defined       no-use-before-define
  263:24  error  'afterWrite' was used before it was defined        no-use-before-define
  265:7   error  'afterWrite' was used before it was defined        no-use-before-define
  275:22  error  'finishMaybe' was used before it was defined       no-use-before-define
  282:5   error  'finishMaybe' was used before it was defined       no-use-before-define
  291:2   error  Unnecessary semicolon                              no-extra-semi
  295:5   error  'onwriteDrain' was used before it was defined      no-use-before-define
  300:3   error  'finishMaybe' was used before it was defined       no-use-before-define
  354:5   error  'finishMaybe' was used before it was defined       no-use-before-define
  392:5   error  Closing curly brace does not appear on the same line
as the subsequent block brace-style
✖ 53 problems (53 errors, 0 warnings)
  9 errors, 0 warnings potentially fixable with the `--fix` option.

machendos

Check the code with eslint

machendos · 2018-08-05T11:44:46Z

lib/array-based/writable.js

+  this._writableState.bufferedArray = null;
+  this._writableState.bufferedSize = 0;
+
+  this._writableState.onwrite = onwrite.bind(undefined, this);


would be better onwrite.bind(null, this);

machendos · 2018-08-05T11:50:56Z

lib/array-based/writable.js

+  state.sync = false;
+};
+
+const onwrite = (stream, error) => {


It is necessary to change all lambda functions to a functional declaration, or to raise lambdas to the top, since they are called above

I'd prefer reorder and use lambdas in all cases where this not needed

machendos · 2018-08-05T12:16:29Z

lib/array-based/writable.js

+  const length = state.objectMode ? 1 : chunk.length;
+  state.length += length;
+
+  let ret = state.length < state.highWaterMark;


const instead of let

machendos · 2018-08-05T12:34:11Z

lib/buffer-based/writable.js

+  delete this._writableState.lastBufferedRequest;
+  delete this._writableState.corkedRequestsFree;
+
+  this._writableState.internalBuffer = Buffer.alloc(this._writableState.highWaterMark);


We can use here Buffer.allocUnsafe

machendos · 2018-08-05T12:51:13Z

lib/array-based/writable.js

+  const callback = state.writecb;
+
+  if (typeof callback !== 'function') {
+    throw new Error('Callback called multiple times');


@tshemsedinov Maybe we should not throw mistakes like this. For example, we can take all the error messages in constants, or at least create variables with them somewhere, and then turn to them? Or is it normal?

For this certain purpose we have callback = common.once(callback) in https://github.com/metarhia/common I do not think we should throw in such cases

lundibundi · 2018-08-05T13:15:31Z

fyi @machendos @SemenchenkoVitaliy this implementation is a somewhat reworked nodejs Writable stream implementation https://github.com/nodejs/node/blob/master/lib/_stream_writable.js.

I believe it's not a good idea to inherit all of that, there is a lot of legacy and even nodejs members want to implement a new stream api that will be much more performant and clean (see links).
On the other hand are there any benchmarks of this, is it event important (will it bring any benefits) to have this in our repo instead of just using plain Writable?

Links:
https://github.com/nodejs/node/projects/7
openjs-foundation/summit#82
https://github.com/Fishrock123/bob

SemenchenkoVitaliy · 2018-08-05T14:47:45Z

Now that I've run speed tests, I see that implementation of @Yone-e is about 1000% slower, maybe due to V8 optimizations of internal streams.

machendos · 2018-08-05T20:17:49Z

The goal of this repository, as I understand it, was to rewrite the usual Writable streams to something faster working, and eventually offer our work in the node. But for now, I dont like this decision very much. I agree with @lundibundi that it's just a little overwritten Writable stream from the node. And I also agree with that we shouldnt inherit this decision. Perhaps someone should address this issue in more detail

lundibundi · 2018-08-25T18:33:32Z

Closing for now due to inactivity. Also refs #6 (comment).

Yone-e added 2 commits June 2, 2018 21:47

Streams: Writable Stream based on Array

85710c7

* implementing 'Writable' which has an Array of Buffers as an internal storage structure

Streams: Writable Stream based on Buffer * implementing 'Writable' wh…

39da3c2

…ich has an Buffer as an internal storage structure

Yone-e force-pushed the review branch from 2134f7b to 39da3c2 Compare June 6, 2018 09:01

tshemsedinov requested review from SemenchenkoVitaliy and machendos August 3, 2018 21:01

SemenchenkoVitaliy requested changes Aug 4, 2018

View reviewed changes

tshemsedinov closed this Aug 4, 2018

tshemsedinov reopened this Aug 4, 2018

machendos requested changes Aug 5, 2018

View reviewed changes

lundibundi closed this Aug 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review #6

Review #6

Yone-e commented Jun 2, 2018

SemenchenkoVitaliy left a comment

SemenchenkoVitaliy Aug 4, 2018 •

edited

Loading

tshemsedinov commented Aug 4, 2018

SemenchenkoVitaliy commented Aug 5, 2018

tshemsedinov commented Aug 5, 2018

machendos left a comment

machendos Aug 5, 2018

machendos Aug 5, 2018

tshemsedinov Aug 5, 2018

machendos Aug 5, 2018

machendos Aug 5, 2018

machendos Aug 5, 2018

tshemsedinov Aug 5, 2018

lundibundi commented Aug 5, 2018

SemenchenkoVitaliy commented Aug 5, 2018

machendos commented Aug 5, 2018

lundibundi commented Aug 25, 2018

Review #6

Review #6

Conversation

Yone-e commented Jun 2, 2018

SemenchenkoVitaliy left a comment

Choose a reason for hiding this comment

SemenchenkoVitaliy Aug 4, 2018 • edited Loading

Choose a reason for hiding this comment

tshemsedinov commented Aug 4, 2018

SemenchenkoVitaliy commented Aug 5, 2018

tshemsedinov commented Aug 5, 2018

machendos left a comment

Choose a reason for hiding this comment

machendos Aug 5, 2018

Choose a reason for hiding this comment

machendos Aug 5, 2018

Choose a reason for hiding this comment

tshemsedinov Aug 5, 2018

Choose a reason for hiding this comment

machendos Aug 5, 2018

Choose a reason for hiding this comment

machendos Aug 5, 2018

Choose a reason for hiding this comment

machendos Aug 5, 2018

Choose a reason for hiding this comment

tshemsedinov Aug 5, 2018

Choose a reason for hiding this comment

lundibundi commented Aug 5, 2018

SemenchenkoVitaliy commented Aug 5, 2018

machendos commented Aug 5, 2018

lundibundi commented Aug 25, 2018

SemenchenkoVitaliy Aug 4, 2018 •

edited

Loading