string_decoder: make write after end to reset #16594

thefourtheye · 2017-10-29T18:18:01Z

When StringDecoder's end is called, it is no longer supposed to wait
for the data. If a write call is made after end, then the decoder
has to be flushed and treated as a brand new write request.

This patch also introduces a new StringDecoder#reset method, which
simply resets all the internal data.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

Fixes: nodejs#16564 When StringDecoder's `end` is called, it is no longer supposed to wait for the data. If a `write` call is made after `end`, then the decoder has to be flushed and treated as a brand new write request. This patch also introduces a new StringDecoder#reset method, which simply resets all the internal data.

vsemozhetbyt · 2017-10-29T18:22:56Z

doc/api/string_decoder.md

@@ -82,3 +82,13 @@ Returns a decoded string, ensuring that any incomplete multibyte characters at
 the end of the `Buffer` are omitted from the returned string and stored in an
 internal buffer for the next call to `stringDecoder.write()` or
 `stringDecoder.end()`.
+
+### stringDecoder.reset([encoding])


A nit: it seems this should go before the stringDecoder.write(), ABC-wise.

mscdex · 2017-10-30T00:46:59Z

lib/string_decoder.js

 StringDecoder.prototype.write = function(buf) {
+  if (this._closed === true)


=== true is unnecessary

If we explicitly compare strictly against the value, the performance would be better right?

Not with TurboFan, no. I asked about this in another PR.

For reference, I think this is the previous discussion in question: #16397 (comment)

mscdex · 2017-10-30T01:05:05Z

What if we do something simpler, like overwrite at least .write() (and possibly .end()) in .end(). Perhaps that would have less overhead? .write() is going to be called more times than .end() in general.

thefourtheye · 2017-10-30T04:13:38Z

@mscdex In that case, users would have to create a new StringDecoder object every time end is called, right? They cannot reuse it. Also, that would break a lot of buggy code which is already out there I think.

mscdex · 2017-10-30T04:26:29Z

@thefourtheye The basic concern still stands though, .write() is hotter than .end(), so if we can come up with some way to avoid adding stuff to .write() that would be ideal (unless benchmarks show that the current solution does not cause a performance regression).

TimothyGu · 2017-10-30T08:00:11Z

doc/api/string_decoder.md

@@ -66,6 +66,16 @@ substitution characters appropriate for the character encoding.
 If the `buffer` argument is provided, one final call to `stringDecoder.write()`
 is performed before returning the remaining input.

+### stringDecoder.reset([encoding])


I believe this is an implementation detail users don't have to know about.

As it is, there is no way for the users to reset the string decoder, right? They have to create a new instance if needed. This will enable reusability.

A new way to reset the state each time was not requested as far as I read the issue. So I also prefer not to expose the reset function. If I am correct the following should work.

const { StringDecoder } = require('string_decoder'); const decoder = new StringDecoder('utf8'); decoder.write(Buffer.from([0xE2, 0x82])); // => '' decoder.end(); // => '�' decoder.write(Buffer.of(0x61)); // => 'a'

TimothyGu · 2017-10-30T08:05:10Z

lib/string_decoder.js

 StringDecoder.prototype.write = function(buf) {
+  if (this._closed === true)
+    this.reset();


Can this logic be in StringDecoder.prototype.end instead? Make the per-encoding this.end switching in the constructor actually set to an internal symbol-named property, and use a shared StringDecoder.prototype.end that calls that internal encoding-specific function and clean up after itself. Would probably help with write performance too if StringDecoders of different encodings (e.g. UTF-8 which does not have an own property end and UTF-16LE that does) are used simultaneously.

jridgewell

This seems all very complicated. Why isn't StringDecoder#end just doing:

this.lastNeed = 0;
this.lastTotal = 0;

The buffer is already "unsafe" and access to it is guarded, so we don't need to sanitize it. And reusing a constructor as a call seems fraught.

mscdex · 2017-10-31T23:27:04Z

This seems all very complicated. Why isn't StringDecoder#end just doing:

One reason is backwards compatibility.

BridgeAR · 2017-11-23T21:55:06Z

Ping @thefourtheye

thefourtheye · 2017-12-01T05:29:01Z

I have updated the PR to make sure that write doesn't have much any changes. PTAL.

mscdex · 2017-12-01T06:37:13Z

lib/string_decoder.js

+function end(buf) {
+  let result = '';
+
+  if (this.encoding === 'utf16le')


Might consider using a switch here.

mscdex · 2017-12-01T06:37:57Z

lib/string_decoder.js

+  let result = '';
+
+  if (this.encoding === 'utf16le')
+    result = utf16End.call(this, buf);


If we're the only ones calling these methods directly, then we should be able to avoid the overhead of .call() and just pass an instance as another argument.

mscdex · 2017-12-01T06:40:55Z

lib/string_decoder.js

+  else
+    result = simpleEnd.call(this, buf);
+
+  this.reset();


Would it be better to use this.reset(this.encoding) to avoid the switch in the constructor?

We still cannot get rid of the switch in the constructor, right?

Correct, but this.reset(this.encoding) would only hit that first if in the constructor, which might be better.

BridgeAR · 2017-12-06T04:34:43Z

lib/string_decoder.js

+]);
+
+function end(buf) {
+  const result = (endMappings.get(this.encoding) || simpleEnd)(this, buf);


This looks "elegant" but using a switch case for the encoding and calling the function directly is probably faster.

BridgeAR · 2017-12-06T04:39:58Z

doc/api/string_decoder.md

@@ -66,6 +66,16 @@ substitution characters appropriate for the character encoding.
 If the `buffer` argument is provided, one final call to `stringDecoder.write()`
 is performed before returning the remaining input.

+### stringDecoder.reset([encoding])


A new way to reset the state each time was not requested as far as I read the issue. So I also prefer not to expose the reset function. If I am correct the following should work.

const { StringDecoder } = require('string_decoder'); const decoder = new StringDecoder('utf8'); decoder.write(Buffer.from([0xE2, 0x82])); // => '' decoder.end(); // => '�' decoder.write(Buffer.of(0x61)); // => 'a'

targos · 2018-01-15T12:05:12Z

ping @thefourtheye

BridgeAR · 2018-01-31T02:02:31Z

Ping again @thefourtheye

This resets the StringDecoder's state after calling `#end`. Further writes to the decoder will act as if it were a brand new instance, allowing simple reuse. Refs: nodejs#16594 Fixes: nodejs#16564

This resets the StringDecoder's state after calling `#end`. Further writes to the decoder will act as if it were a brand new instance, allowing simple reuse. PR-URL: #18494 Fixes: #16564 Refs: #16594 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

This resets the StringDecoder's state after calling `#end`. Further writes to the decoder will act as if it were a brand new instance, allowing simple reuse. PR-URL: nodejs#18494 Fixes: nodejs#16564 Refs: nodejs#16594 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

nodejs-github-bot added the string_decoder Issues and PRs related to the string_decoder subsystem. label Oct 29, 2017

thefourtheye mentioned this pull request Oct 29, 2017

StringDecoder#end doesn't flush state #16564

Closed

vsemozhetbyt reviewed Oct 29, 2017

View reviewed changes

mscdex reviewed Oct 30, 2017

View reviewed changes

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 30, 2017

sort the doc section

dbf8fd2

TimothyGu reviewed Oct 30, 2017

View reviewed changes

jridgewell suggested changes Oct 31, 2017

View reviewed changes

make write hot path efficient

f34b251

mscdex reviewed Dec 1, 2017

View reviewed changes

incorporate review comments

7c2286f

thefourtheye force-pushed the string-decoder-fix branch from fe260cc to 7c2286f Compare December 5, 2017 08:49

BridgeAR reviewed Dec 6, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

jridgewell mentioned this pull request Feb 1, 2018

string_decoder: reset decoder on end #18494

Closed

3 tasks

thefourtheye closed this Feb 1, 2018

thefourtheye deleted the string-decoder-fix branch February 1, 2018 09:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

string_decoder: make write after end to reset #16594

string_decoder: make write after end to reset #16594

thefourtheye commented Oct 29, 2017

vsemozhetbyt Oct 29, 2017

thefourtheye Oct 30, 2017

mscdex Oct 30, 2017

thefourtheye Oct 30, 2017

mscdex Oct 30, 2017

cjihrig Oct 30, 2017

mscdex commented Oct 30, 2017

thefourtheye commented Oct 30, 2017

mscdex commented Oct 30, 2017

TimothyGu Oct 30, 2017

thefourtheye Dec 1, 2017

BridgeAR Dec 6, 2017

TimothyGu Oct 30, 2017

jridgewell left a comment

mscdex commented Oct 31, 2017 •

edited

Loading

BridgeAR commented Nov 23, 2017

thefourtheye commented Dec 1, 2017

mscdex Dec 1, 2017

thefourtheye Dec 5, 2017

mscdex Dec 1, 2017

thefourtheye Dec 5, 2017

mscdex Dec 1, 2017

thefourtheye Dec 5, 2017

mscdex Dec 5, 2017 •

edited

Loading

BridgeAR Dec 6, 2017

BridgeAR Dec 6, 2017

targos commented Jan 15, 2018

BridgeAR commented Jan 31, 2018

		StringDecoder.prototype.write = function(buf) {
		if (this._closed === true)

string_decoder: make write after end to reset #16594

string_decoder: make write after end to reset #16594

Conversation

thefourtheye commented Oct 29, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Oct 30, 2017

thefourtheye commented Oct 30, 2017

mscdex commented Oct 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell left a comment

Choose a reason for hiding this comment

mscdex commented Oct 31, 2017 • edited Loading

BridgeAR commented Nov 23, 2017

thefourtheye commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Jan 15, 2018

BridgeAR commented Jan 31, 2018

mscdex commented Oct 31, 2017 •

edited

Loading

mscdex Dec 5, 2017 •

edited

Loading