Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: refactor internal/util #11404

Closed
wants to merge 3 commits into from

Conversation

@jasnell
Copy link
Member

commented Feb 15, 2017

  • 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

@jasnell jasnell force-pushed the jasnell:internal-util-refactor branch Feb 15, 2017

@jasnell jasnell changed the title src: refactor internal/util lib: refactor internal/util Feb 15, 2017

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2017

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

@sam-github

This comment has been minimized.

Copy link
Member

commented Feb 16, 2017

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

@sam-github

This comment has been minimized.

Copy link
Member

commented Feb 16, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2017

@vsemozhetbyt vsemozhetbyt referenced this pull request Feb 17, 2017
2 of 2 tasks complete

@jasnell jasnell force-pushed the jasnell:internal-util-refactor branch Feb 17, 2017

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2017

@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

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2017

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

@jasnell jasnell force-pushed the jasnell:internal-util-refactor branch 3 times, most recently Feb 19, 2017

@fhalde
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2017

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

@refack refack force-pushed the nodejs:master branch to fbe946b Apr 14, 2017

lib: refactor internal/util
* Use the more efficient module.exports = {} approach
* Eliminate some uses of arguments

@jasnell jasnell force-pushed the jasnell:internal-util-refactor branch to bf29544 Apr 20, 2017

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2017

@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

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2017

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2017

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

@@ -186,4 +197,27 @@ exports.convertToValidSignal = function convertToValidSignal(signal) {
}

throw new Error('Unknown signal: ' + signal);
}

module.exports = {

This comment has been minimized.

Copy link
@sam-github

sam-github Apr 20, 2017

Member

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.

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 20, 2017

Author Member

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.

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 20, 2017

Contributor

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.

function isError(e) {
return objectToString(e) === '[object Error]' || e instanceof Error;
}

This comment has been minimized.

Copy link
@sam-github

sam-github Apr 20, 2017

Member

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

@sam-github
Copy link
Member

left a comment

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

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2017

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

@sam-github

This comment has been minimized.

Copy link
Member

commented Apr 20, 2017

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

return Object.prototype.toString.call(o);
}

// Mark that a method should not be used.

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 20, 2017

Contributor

These comments shouldn't be indented.

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 20, 2017

Author Member

whoops! good catch :-)

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2017

Updated!

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2017

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2017

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 added a commit that referenced this pull request Apr 21, 2017
lib: refactor internal/util
* Use the more efficient module.exports = {} approach
* Eliminate some uses of arguments

PR-URL: #11404
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net
@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2017

Landed in 9077b48

@jasnell jasnell closed this Apr 21, 2017

@TimothyGu TimothyGu referenced this pull request Apr 22, 2017
1 of 1 task complete
@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 18, 2017

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

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2017

Only if there's no negative perf hit.

@gibfahn gibfahn added land-on-v6.x and removed lts-watch-v6.x labels Jun 19, 2017

@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 19, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.