lib: refactor internal/util

[jasnell]

• Use the more efficient module.exports ={} approach
• Eliminate some uses of arguments

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

internal/util

[mscdex]

One thing to keep in mind when inlining functions inside module.exports ={... } is that the extra indentation will increase the function body length, which will cause issues with runtime inlineability if the new function's length is >= 600 characters.

[jasnell]

Yeah i was thinking about that. Just in general it would be good to move the function bodies out of the object.

[sam-github]

I'm not sure the refactor makes it better, just stylistically different. What is more efficient? There are benchmarks for this?

[sam-github]

modules that do exports.whatever = ... through-out are common style in js, easy to understand, familiar to any js develeoper, and used widely in node.js lib/, rewriting will make backporting across this barrier pretty hard, and its just moving from one of the two most popular js module styles to the other.

I don't think this kind of sweeping stylistic change is worth doing unless its really compellingly better style or has some compelling functional difference and I'm not sure the "more efficient" style will be noticeable.

If we didn't have to backport, I'd have no particular opinion, but sweeping stylistic changes that make backports hard worries me.

In contrast, breaking up the absolutely enormous lib/crypto.js is, I think, compellingly better style, easier to read, maintain, etc., so despite the backport cost, I was +1 for that. Here, its not so clear to me.

[vsemozhetbyt]

@sam-github FWIW: #11430

[jasnell]

@mscdex@sam-github ... so this one we will definitely want to investigate further and hold off on landing. Unlike the other similar changes I've done moving to the module.exports ={} pattern (which have shown about a 5% performance improvement), the change in this module shows a significant perf regression when running in a benchmark test.

[jasnell]

I've added the in progress label to ensure this doesn't get landed before we complete the investigation

[fhalde]

Not sure what is the general practice followed in the node code base. But looks like you've been missing out on semicolons after end of the function block. Looking at the history of the file, function blocks were ended with semicolons.

[jasnell]

semicolons are not required to closing function blocks for regular functions with our linting rules. They are only required on assignments.

[jasnell]

@mscdex@sam-github ... I've updated this PR. The perf diff with master compared to 7.9 is negligible.

The following is the benchmark comparison results for the normalize encoding method in internal/util:

Master to 7.9

 improvement confidence p.value util/normalize-encoding.js n=100000 input="" -4.45 % 4.711075e-01 util/normalize-encoding.js n=100000 input="[]" 36.27 % *** 2.548943e-08 util/normalize-encoding.js n=100000 input="1" -5.27 % 3.436246e-01 util/normalize-encoding.js n=100000 input="base64" 3.10 % 6.309801e-01 util/normalize-encoding.js n=100000 input="BASE64" -7.69 % 1.484294e-01 util/normalize-encoding.js n=100000 input="binary" 3.06 % 6.140675e-01 util/normalize-encoding.js n=100000 input="BINARY" -5.41 % 3.480641e-01 util/normalize-encoding.js n=100000 input="false" 2.86 % 7.025918e-01 util/normalize-encoding.js n=100000 input="foo" 2.63 % 6.799095e-01 util/normalize-encoding.js n=100000 input="group_common" -15.97 % ** 2.509445e-03 util/normalize-encoding.js n=100000 input="group_misc" -5.11 % 2.732518e-01 util/normalize-encoding.js n=100000 input="group_uncommon" -16.40 % ** 2.064198e-03 util/normalize-encoding.js n=100000 input="group_upper" -7.97 % * 4.742396e-02 util/normalize-encoding.js n=100000 input="hex" 2.49 % 6.976260e-01 util/normalize-encoding.js n=100000 input="HEX" -0.64 % 9.160260e-01 util/normalize-encoding.js n=100000 input="latin1" -0.11 % 9.873016e-01 util/normalize-encoding.js n=100000 input="ucs2" -4.21 % 5.362670e-01 util/normalize-encoding.js n=100000 input="UCS2" -12.25 % * 3.123020e-02 util/normalize-encoding.js n=100000 input="undefined" -5.21 % 4.693525e-01 util/normalize-encoding.js n=100000 input="utf-16le" 7.69 % 3.099749e-01 util/normalize-encoding.js n=100000 input="UTF-16LE" 5.83 % 2.410658e-01 util/normalize-encoding.js n=100000 input="utf-8" -4.69 % 4.729617e-01 util/normalize-encoding.js n=100000 input="utF-8" 1.09 % 8.107947e-01 util/normalize-encoding.js n=100000 input="uTf-8" -0.05 % 9.919185e-01 util/normalize-encoding.js n=100000 input="UTF-8" -2.07 % 6.396208e-01 util/normalize-encoding.js n=100000 input="utf16le" -3.86 % 5.127621e-01 util/normalize-encoding.js n=100000 input="UTF16LE" -5.05 % 3.387483e-01 util/normalize-encoding.js n=100000 input="utf8" -6.97 % 2.784592e-01 util/normalize-encoding.js n=100000 input="Utf8" -10.27 % * 3.095776e-02 util/normalize-encoding.js n=100000 input="UTF8" -1.84 % 6.712303e-01 

This PR to 7.9

 improvement confidence p.value util/normalize-encoding.js n=100000 input="" 3.55 % 6.846831e-01 util/normalize-encoding.js n=100000 input="[]" 46.68 % *** 2.736654e-09 util/normalize-encoding.js n=100000 input="1" 4.98 % 3.766843e-01 util/normalize-encoding.js n=100000 input="base64" -0.16 % 9.787752e-01 util/normalize-encoding.js n=100000 input="BASE64" 3.66 % 5.249255e-01 util/normalize-encoding.js n=100000 input="binary" 8.36 % 1.699122e-01 util/normalize-encoding.js n=100000 input="BINARY" 7.63 % 1.570101e-01 util/normalize-encoding.js n=100000 input="false" 10.93 % 1.812432e-01 util/normalize-encoding.js n=100000 input="foo" -3.55 % 4.628670e-01 util/normalize-encoding.js n=100000 input="group_common" -21.83 % ** 3.009091e-03 util/normalize-encoding.js n=100000 input="group_misc" -7.03 % 2.180318e-01 util/normalize-encoding.js n=100000 input="group_uncommon" -9.68 % 9.350889e-02 util/normalize-encoding.js n=100000 input="group_upper" -4.28 % 4.121715e-01 util/normalize-encoding.js n=100000 input="hex" 8.06 % 1.020501e-01 util/normalize-encoding.js n=100000 input="HEX" -6.09 % 2.984084e-01 util/normalize-encoding.js n=100000 input="latin1" 14.54 % 6.504796e-02 util/normalize-encoding.js n=100000 input="ucs2" -5.27 % 4.170471e-01 util/normalize-encoding.js n=100000 input="UCS2" -13.12 % * 3.303378e-02 util/normalize-encoding.js n=100000 input="undefined" 2.53 % 7.443536e-01 util/normalize-encoding.js n=100000 input="utf-16le" 5.47 % 4.676833e-01 util/normalize-encoding.js n=100000 input="UTF-16LE" 5.10 % 3.996346e-01 util/normalize-encoding.js n=100000 input="utf-8" -5.90 % 4.555466e-01 util/normalize-encoding.js n=100000 input="utF-8" -8.04 % 1.263448e-01 util/normalize-encoding.js n=100000 input="uTf-8" -0.35 % 9.586554e-01 util/normalize-encoding.js n=100000 input="UTF-8" -8.36 % 1.802233e-01 util/normalize-encoding.js n=100000 input="utf16le" 0.59 % 9.322743e-01 util/normalize-encoding.js n=100000 input="UTF16LE" -6.19 % 2.138244e-01 util/normalize-encoding.js n=100000 input="utf8" -10.47 % 1.785193e-01 util/normalize-encoding.js n=100000 input="Utf8" -6.53 % 3.446999e-01 util/normalize-encoding.js n=100000 input="UTF8" -7.60 % 2.526299e-01 

PTAL

[jasnell]

CI: https://ci.nodejs.org/job/node-test-pull-request/7558/

[mscdex]

You can actually remove toInteger() and toLength() completely, they're not used anywhere anymore.

[sam-github]

assigning to exports = as well is more future proof, it allows util functions to refer to each other more easily.

Of the 3 common export styles, at-top, at-definition (what util used to do), and at-bottom,at-bottom remains my least favorite. It lacks the visibility and readability of export at top, and lacks the cohesion of export at time of definition. But if at-bottom is how node.js goes, I'll follow along.

[jasnell]

Avoiding the need to refer to exports.{whatever} is important and is why the various exported functions are all named top level functions (they can refer to each other without the exports. bit). That said, it's harmless to add.

[mscdex]

One problem with having it at the top is that you can run into variable definition issues, where you're exporting something that gets defined/set later on in the file. Sometimes you might be able to just pull the definition to the top, but other times the assignment may not be a simple one-liner. So putting it at the bottom means you never have to worry about such issues and doing it the same everywhere is good for consistency.

[sam-github]

sometimes there is two lines beween definitions, sometimes one, should probably be consisten

[sam-github]

AFAICT from the diff, this is adding new APIs to util, toInteger() and toLength(). Am I misreading?

[jasnell]

The toInteger() and toLength() bits were remnants from the original version of this. Those were removed subsequently but did not get deleted after the rebase. I'm removing them.

[jasnell]

PR Updated to remove the toInteger() and toLength(). PTAL

[sam-github]

I would prefer for both exports and module.exports to be reassigned, to keep a consistent idiom (and I think it costs nothing). Other than that, LGTM

[mscdex]

These comments shouldn't be indented.

[jasnell]

whoops! good catch :-)

[jasnell]

Updated!

[mscdex]

LGTM if CI is ok with it: https://ci.nodejs.org/job/node-test-pull-request/7560/

[jasnell]

Fedora on CI has been a bit challenged today in terms of speed, but everything else looks good. Will get this landed tomorrow assuming there are no issues.

[jasnell]

Landed in 9077b48

[gibfahn]

@jasnell is this something we want on v6.x?

[jasnell]

Only if there's no negative perf hit.

[gibfahn]

Okay, I'm marking this as don't land, but if anyone is willing to backport (and test the perf) on this then go for it.