string_decoder: make write after end to reset by thefourtheye · Pull Request #16594 · nodejs/node · GitHub

Fixes: #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.

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.

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

Done!

=== 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)

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.

@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.

@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).

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'

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.

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.

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

One reason is backwards compatibility.

Ping @thefourtheye

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

Might consider using a switch here.

Ack.

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.

Ack.

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.

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

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'

ping @thefourtheye

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 <timothygu99@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

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 <timothygu99@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

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

               internal buffer for the next call to `stringDecoder.write()` or
               `stringDecoder.end()`.
+              ### stringDecoder.reset([encoding])

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

               If the `buffer` argument is provided, one final call to `stringDecoder.write()`
               is performed before returning the remaining input.
+              ### stringDecoder.reset([encoding])

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

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

string_decoder: make write after end to reset by thefourtheye · Pull Request #16594 · nodejs/node · GitHub

Uh oh!

Uh oh!

string_decoder: make write after end to reset - #16594

string_decoder: make write after end to reset#16594thefourtheye wants to merge 4 commits intonodejs:masternodejs/node:masterfrom thefourtheye:string-decoder-fixthefourtheye/io.js:string-decoder-fixCopy head branch name to clipboard

Conversation

thefourtheye commented Oct 29, 2017

Checklist

Affected core subsystem(s)

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!

mscdex commented Oct 30, 2017

Uh oh!

thefourtheye commented Oct 30, 2017

Uh oh!

mscdex commented Oct 30, 2017

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!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

mscdex commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Nov 23, 2017

Uh oh!

thefourtheye commented Dec 1, 2017

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!

mscdex Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

targos commented Jan 15, 2018

Uh oh!

BridgeAR commented Jan 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

string_decoder: make write after end to reset#16594
thefourtheye wants to merge 4 commits into
nodejs:masterfrom
thefourtheye:string-decoder-fix

mscdex commented Oct 31, 2017 •
edited
Loading

mscdex Dec 5, 2017 •
edited
Loading