FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

util: improve readability of normalizeEncoding by joyeecheung · Pull Request #10439 · nodejs/node · GitHub

/ node Public

util: improve readability of normalizeEncoding - #10439

Closed
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:refactor-normalize-encoding
ConversationCommits (1)ChecksFiles changed
Closed

util: improve readability of normalizeEncoding#10439
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:refactor-normalize-encoding

Conversation

joyeecheung commented Dec 24, 2016 •
edited
Loading

Copy link
Copy Markdown
Member
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

util

Description of change

Replace the redundant for loop with if-else.

EDIT: there is a inevitable performance hit if rewrite the loop into if-else with a helper function. Rewrite to use a slightly more readable while loop with comments.

Also add a benchmark for util.normalizeEncoding

nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Dec 24, 2016

joyeecheung commented Dec 24, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

The performance impact is mixed(inputs taken from the test-internal-util-formatencoding test cases). It's not in the critical path anyway I think (mostly used in configuration phases / when user provide a uncommon encoding).

The benchmark infra is not ready to do internal module benchmarks. Not sure if putting this on the external util just for benchmarking purpose is a good idea, so I didn't add the benchmarking code to this PR.

'use strict';
const common = require('../common.js');
const normalizeEncoding = require('util')._normalizeEncoding;

const inputs = ['', 'utf8', 'utf-8', 'UTF-8',
  'UTF8', 'Utf8', 'uTf-8', 'utF-8', 'ucs2',
  'UCS2', 'utf16le', 'utf-16le', 'UTF-16LE', 'UTF16LE',
  'binary', 'BINARY', 'latin1', 'base64', 'BASE64',
  'hex', 'HEX', 'foo', 1, false, undefined, []];

const bench = common.createBenchmark(main, {
  index: Object.keys(inputs).map((key) => parseInt(key)),
  n: [1e5]
});

function main(conf) {
  const index = conf.index;
  const n = conf.n | 0;

  const input = inputs[index] || '';

  common.v8ForceOptimization(normalizeEncoding, 'utf8');

  bench.start();
  for (var i = 0; i < n; i += 1)
    normalizeEncoding(input);
  bench.end(n);
}
See benchmark results 
                                              improvement significant      p.value
 util/normalize-encoding.js n=100000 index=0      -9.32 %         *** 2.594961e-04
 util/normalize-encoding.js n=100000 index=1     -13.30 %         *** 1.148799e-04
 util/normalize-encoding.js n=100000 index=10     -6.37 %           * 1.817863e-02
 util/normalize-encoding.js n=100000 index=11     -3.93 %             1.070895e-01
 util/normalize-encoding.js n=100000 index=12     -1.12 %             5.513625e-01
 util/normalize-encoding.js n=100000 index=13      1.21 %             5.917431e-01
 util/normalize-encoding.js n=100000 index=14     10.52 %         *** 2.672550e-05
 util/normalize-encoding.js n=100000 index=15     -3.98 %           * 1.027313e-02
 util/normalize-encoding.js n=100000 index=16      2.20 %             3.329777e-01
 util/normalize-encoding.js n=100000 index=17     14.93 %         *** 7.643643e-07
 util/normalize-encoding.js n=100000 index=18     -3.97 %           * 1.168600e-02
 util/normalize-encoding.js n=100000 index=19     26.33 %         *** 5.501596e-17
 util/normalize-encoding.js n=100000 index=2     -19.27 %         *** 4.589156e-10
 util/normalize-encoding.js n=100000 index=20     -3.80 %           * 1.572212e-02
 util/normalize-encoding.js n=100000 index=21      4.02 %           * 3.897232e-02
 util/normalize-encoding.js n=100000 index=22     27.05 %         *** 3.436555e-19
 util/normalize-encoding.js n=100000 index=23    -10.16 %         *** 4.974043e-06
 util/normalize-encoding.js n=100000 index=24    -16.14 %         *** 1.010719e-06
 util/normalize-encoding.js n=100000 index=25      6.56 %         *** 1.258479e-05
 util/normalize-encoding.js n=100000 index=3      -0.35 %             8.880709e-01
 util/normalize-encoding.js n=100000 index=4      -2.04 %             2.544653e-01
 util/normalize-encoding.js n=100000 index=5      -2.06 %             2.306798e-01
 util/normalize-encoding.js n=100000 index=6      -1.51 %             3.367092e-01
 util/normalize-encoding.js n=100000 index=7      -0.31 %             8.605040e-01
 util/normalize-encoding.js n=100000 index=8     -11.23 %         *** 6.449605e-06
 util/normalize-encoding.js n=100000 index=9      -0.16 %             9.223133e-01

mscdex commented Dec 24, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

About the benchmark: adding .sort() after .map() will make the results easier to follow, also the common.v8ForceOptimization() should be removed because the functions should be inlineable and calling common.v8ForceOptimization() causes the passed function to not be inlined.

Re-post benchmark results after making those changes and let's see where we stand.

joyeecheung commented Dec 25, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

@mscdex I've discovered that internal modules can be benchmarked if you pass the --expose-internals flag to node compare.js(all benchmarks run will "inherits" these flags), so I will push those benchmarks in this PR too.

Below are the new results, these are quite different from the previous ones. I find it a little bit strange that the cases with the most regressions are the ones that should be left untouched in the first line (false, undefined, .etc). I'll see what I can find from the asm(or use a do-while loop if it has to be implemented this way, at least do-while is a more readable than for(;;) IMHO).

See benchmark results 
                                                        improvement significant      p.value
 util/normalize-encoding.js n=1000000 input=""             -47.80 %         *** 9.666964e-30
 util/normalize-encoding.js n=1000000 input="1"             33.18 %         *** 1.848349e-25
 util/normalize-encoding.js n=1000000 input="BASE64"         0.99 %             1.996811e-01
 util/normalize-encoding.js n=1000000 input="BINARY"        -0.52 %             6.046694e-01
 util/normalize-encoding.js n=1000000 input="HEX"           -1.17 %             5.406674e-01
 util/normalize-encoding.js n=1000000 input="UCS2"           4.44 %         *** 1.386089e-06
 util/normalize-encoding.js n=1000000 input="UTF-16LE"       0.50 %             5.726062e-01
 util/normalize-encoding.js n=1000000 input="UTF-8"          0.70 %             4.408422e-01
 util/normalize-encoding.js n=1000000 input="UTF16LE"        0.51 %             6.153607e-01
 util/normalize-encoding.js n=1000000 input="UTF8"           4.80 %         *** 4.519778e-07
 util/normalize-encoding.js n=1000000 input="Utf8"           2.25 %           * 1.944775e-02
 util/normalize-encoding.js n=1000000 input="[]"            10.21 %         *** 2.153273e-16
 util/normalize-encoding.js n=1000000 input="base64"         0.28 %             8.131951e-01
 util/normalize-encoding.js n=1000000 input="binary"         0.13 %             9.215771e-01
 util/normalize-encoding.js n=1000000 input="false"        -54.78 %         *** 2.363131e-38
 util/normalize-encoding.js n=1000000 input="foo"            1.90 %             7.937867e-02
 util/normalize-encoding.js n=1000000 input="hex"           -3.71 %          ** 2.582364e-03
 util/normalize-encoding.js n=1000000 input="latin1"        -2.51 %             1.176542e-01
 util/normalize-encoding.js n=1000000 input="uTf-8"          0.94 %             3.604232e-01
 util/normalize-encoding.js n=1000000 input="ucs2"          -2.02 %             2.298876e-01
 util/normalize-encoding.js n=1000000 input="undefined"    -56.19 %         *** 1.454327e-27
 util/normalize-encoding.js n=1000000 input="utF-8"          1.91 %             2.511025e-01
 util/normalize-encoding.js n=1000000 input="utf-16le"      -2.60 %             6.766127e-02
 util/normalize-encoding.js n=1000000 input="utf-8"         -3.06 %             1.346169e-01
 util/normalize-encoding.js n=1000000 input="utf16le"       -4.05 %           * 1.658632e-02
 util/normalize-encoding.js n=1000000 input="utf8"         -11.40 %         *** 1.452832e-08
```

joyeecheung commented Dec 25, 2016

Copy link
Copy Markdown
Member Author

BTW: I'll try to submit another PR to make it possible to use comments to indicate the flags(like what we have in tests, // Flags: --expose-internals)

mscdex commented Dec 25, 2016

Copy link
Copy Markdown
Contributor

@joyeecheung The benchmark runner should already be passing that flag to all benchmarks, assuming you're referring to benchmarks.

mscdex commented Dec 25, 2016

Copy link
Copy Markdown
Contributor

The 'utf8' input perf hit is troubling. If the benchmarks are finishing too quickly, you may want to bump up the default value of n to make the functions run for a while (to give V8 time to compile, optimize, etc.).

joyeecheung mentioned this pull request Dec 25, 2016
Closed
3 tasks

joyeecheung commented Dec 25, 2016

Copy link
Copy Markdown
Member Author

@mscdex It actually works like this. If we don't pass in --expose-internals and require the module before main is run, the benchmark still won't be able to find the modules.

joyeecheung commented Dec 25, 2016

Copy link
Copy Markdown
Member Author

@mscdex Thanks for the advice, I'll change the benchmark code to see if bumping n for fast-return arguments lead to different results.

joyeecheung commented Dec 26, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

So, I've bumped the n for the fast-return arguments and tried to group the inputs so that each benchmark would receive more than one values, but the results are still very similar to the one I posted earlier.

One thing I notice is that the control graph looks so much cleaner in the previous implementation. I'll spend a bit more time on this to see if this is a v8 bug.

See control flow graph

Old:

New:

joyeecheung force-pushed the refactor-normalize-encoding branch 2 times, most recently from ab27388 to fccd403 Compare January 3, 2017 12:01

joyeecheung commented Jan 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

So I've changed the benchmarks a bit to feed more than one type of input in each run, and added some code to avoid quirks in micro benchmarks. The performance hit is much smaller when the inputs are fed in groups (notice the group_* inputs). I guess the performance loss observed for ungrouped inputs comes from the trickiness of micro benchmarks?

See benchmark results> 
                                                            improvement significant      p.value
 util/normalize-encoding.js n=100000 input=""                  -19.94 %         *** 1.057313e-14
 util/normalize-encoding.js n=100000 input="1"                  24.89 %         *** 3.630534e-16
 util/normalize-encoding.js n=100000 input="BASE64"             -2.73 %             1.072942e-01
 util/normalize-encoding.js n=100000 input="BINARY"             -6.57 %          ** 4.003795e-03
 util/normalize-encoding.js n=100000 input="HEX"                -4.36 %          ** 4.988303e-03
 util/normalize-encoding.js n=100000 input="UCS2"               -0.41 %             8.503550e-01
 util/normalize-encoding.js n=100000 input="UTF-16LE"           -3.93 %           * 2.017258e-02
 util/normalize-encoding.js n=100000 input="UTF-8"              -3.39 %             6.457016e-02
 util/normalize-encoding.js n=100000 input="UTF16LE"            -4.98 %          ** 1.586828e-03
 util/normalize-encoding.js n=100000 input="UTF8"               -1.70 %             2.326543e-01
 util/normalize-encoding.js n=100000 input="Utf8"               -3.09 %             1.222550e-01
 util/normalize-encoding.js n=100000 input="[]"                  5.97 %         *** 5.884119e-06
 util/normalize-encoding.js n=100000 input="base64"             -2.97 %             1.532578e-01
 util/normalize-encoding.js n=100000 input="binary"             -5.56 %          ** 2.266815e-03
 util/normalize-encoding.js n=100000 input="false"             -25.80 %         *** 5.718812e-23
 util/normalize-encoding.js n=100000 input="foo"                -2.59 %             1.255741e-01
 util/normalize-encoding.js n=100000 input="group_common"       -1.94 %             2.243914e-01
 util/normalize-encoding.js n=100000 input="group_misc"          0.21 %             8.978335e-01
 util/normalize-encoding.js n=100000 input="group_uncommon"      7.07 %         *** 1.097578e-05
 util/normalize-encoding.js n=100000 input="group_upper"        -1.90 %             1.982519e-01
 util/normalize-encoding.js n=100000 input="hex"                -0.57 %             7.268864e-01
 util/normalize-encoding.js n=100000 input="latin1"             -4.75 %           * 1.667714e-02
 util/normalize-encoding.js n=100000 input="uTf-8"              -3.49 %           * 2.158359e-02
 util/normalize-encoding.js n=100000 input="ucs2"              -13.06 %         *** 1.131472e-10
 util/normalize-encoding.js n=100000 input="undefined"         -22.77 %         *** 2.384209e-14
 util/normalize-encoding.js n=100000 input="utF-8"              -3.45 %           * 3.785455e-02
 util/normalize-encoding.js n=100000 input="utf-16le"           -5.99 %          ** 7.161187e-03
 util/normalize-encoding.js n=100000 input="utf-8"             -10.35 %         *** 4.765813e-08
 util/normalize-encoding.js n=100000 input="utf16le"            -9.41 %         *** 2.643782e-08
 util/normalize-encoding.js n=100000 input="utf8"              -12.76 %         *** 7.559608e-09

mscdex commented Jan 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Most likely the main issue is the extra function calls. The loop is what avoids that in the original implementation.

I'm still concerned about the performance degradation.

joyeecheung commented Jan 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

@mscdex From what I've seen in the HIR it's probably not about the function call itself(because they would be inlined anyway), but about normalize here being a "closure" (see v8 bug) which is kinda inevitable given the way Node wraps around modules. The effect of those extra instructions is amplified in those fast-return cases, but not so obvious in the no-so-fast/mixed cases.

See HIR

Old:

    begin_HIR
      0 0 v339 BlockEntry  type:Tagged pos:2_48 <|@
      0 0 v340 Simulate id=37 type:Tagged pos:2_48 <|@
      0 0 v341 StackCheck t310 changes[NewSpacePromotion] type:Tagged pos:2_48 <|@
      0 0 v345 Simulate id=60 var[3] = t573, push t329 type:Tagged pos:2_63 <|@
      0 0 t347 CheckHeapObject t329 pos:2_63 <|@
      0 0 t348 CheckInstanceType internalized_string t329 pos:2_63 <|@
      0 0 v350 CompareObjectEqAndBranch t329 t324 goto (B63, B40) type:Tagged pos:2_63 <|@
    end_HIR

New

    begin_HIR
      0 0 v328 BlockEntry  type:Tagged pos:2_56 <|@
      0 4 t329 LoadContextSlot t310[9] type:Tagged pos:2_56 <|@
      0 0 t330 CheckValue t329 0x30917856f799  type:Tagged pos:2_56 <|@
      0 0 t332 ArgumentsObject t3 t308 type:Tagged pos:3_0 <|@
      0 0 v333 Simulate id=-1 push t329, push t3, push t308 type:Tagged pos:3_0 <|@
      0 0 v334 EnterInlined normalize type:Tagged pos:3_0 <|@
      0 0 v338 Simulate id=24 var[3] = t578, var[2] = t310, push t308 type:Tagged pos:3_10 <|@
      0 0 t340 CheckHeapObject t308 pos:3_10 <|@
      0 0 t341 CheckInstanceType internalized_string t308 pos:3_10 <|@
      0 0 v343 CompareObjectEqAndBranch t308 t324 goto (B75, B38) type:Tagged pos:3_10 <|@
    end_HIR

Since that bug is not going to be fixed anytime soon I'll try to use do-while instead.

EDIT: that bug seems to be worked on as another V8 bug. But anyways...

joyeecheung force-pushed the refactor-normalize-encoding branch from fccd403 to d02dedb Compare January 7, 2017 09:50

joyeecheung commented Jan 7, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

I have updated the code to use a while loop instead with some comments on top(can't comment in the function because then it will be over max_inlined_source_size). The benchmark results look OK but the non-grouped ones are kinda flaky. Sometimes they can be +1% ish and sometimes down to -9% (I don't have access to a really stable environment to run this so..not very sure about this :/). I guess the flakiness is just positively correlated to the micro-ness of these benchmarks..

See benchmark results 
                                                            improvement significant      p.value
 util/normalize-encoding.js n=100000 input=""                    1.48 %             0.0829767494
 util/normalize-encoding.js n=100000 input="1"                  -0.46 %             0.6432198748
 util/normalize-encoding.js n=100000 input="BASE64"             -0.40 %             0.6207269559
 util/normalize-encoding.js n=100000 input="BINARY"             -0.31 %             0.6789723120
 util/normalize-encoding.js n=100000 input="HEX"                -1.38 %             0.1882772287
 util/normalize-encoding.js n=100000 input="UCS2"               -1.40 %             0.1642200780
 util/normalize-encoding.js n=100000 input="UTF-16LE"            0.21 %             0.9227927195
 util/normalize-encoding.js n=100000 input="UTF-8"              -0.28 %             0.7744125407
 util/normalize-encoding.js n=100000 input="UTF16LE"            -2.54 %          ** 0.0049099170
 util/normalize-encoding.js n=100000 input="UTF8"                0.41 %             0.7577822666
 util/normalize-encoding.js n=100000 input="Utf8"               -1.62 %           * 0.0166714587
 util/normalize-encoding.js n=100000 input="[]"                  0.12 %             0.8908627898
 util/normalize-encoding.js n=100000 input="base64"             -0.34 %             0.7674064864
 util/normalize-encoding.js n=100000 input="binary"             -1.28 %             0.2938272124
 util/normalize-encoding.js n=100000 input="false"               0.06 %             0.9413756246
 util/normalize-encoding.js n=100000 input="foo"                -2.31 %         *** 0.0001253098
 util/normalize-encoding.js n=100000 input="group_common"        0.01 %             0.9897426571
 util/normalize-encoding.js n=100000 input="group_misc"         -2.79 %          ** 0.0041665424
 util/normalize-encoding.js n=100000 input="group_uncommon"     -0.15 %             0.8362969697
 util/normalize-encoding.js n=100000 input="group_upper"        -2.34 %          ** 0.0043682302
 util/normalize-encoding.js n=100000 input="hex"                 1.29 %             0.3013361526
 util/normalize-encoding.js n=100000 input="latin1"             -0.44 %             0.7223575589
 util/normalize-encoding.js n=100000 input="uTf-8"              -1.35 %             0.5255336214
 util/normalize-encoding.js n=100000 input="ucs2"                0.31 %             0.7994968617
 util/normalize-encoding.js n=100000 input="undefined"          -0.17 %             0.9115657865
 util/normalize-encoding.js n=100000 input="utF-8"              -0.30 %             0.7290963708
 util/normalize-encoding.js n=100000 input="utf-16le"           -2.80 %         *** 0.0009771347
 util/normalize-encoding.js n=100000 input="utf-8"               1.05 %             0.2556790884
 util/normalize-encoding.js n=100000 input="utf16le"            -1.10 %             0.4710466247
 util/normalize-encoding.js n=100000 input="utf8"                0.76 %             0.3382814236

joyeecheung force-pushed the refactor-normalize-encoding branch from d02dedb to a9681a1 Compare January 7, 2017 12:49
joyeecheung added the benchmark Issues and PRs related to the benchmark subsystem. label Jan 7, 2017

joyeecheung commented Jan 10, 2017

Copy link
Copy Markdown
Member Author

@mscdex Does this LGTY?

mscdex commented Jan 10, 2017

Copy link
Copy Markdown
Contributor

I'm still concerned about the performance. Most of the results are not marked as highly significant and the ones that are show a performance regression. I'm just not sure it's worth it just to change the loop used.

Did you try benchmarking current master against itself to see if similar results emerge?

joyeecheung commented Jan 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

@mscdex I ran them again an this time there is no significant p value...strange...

Against this PR:

See benchmark results 
                                                            improvement significant    p.value
 util/normalize-encoding.js n=100000 input=""                    0.76 %             0.78083843
 util/normalize-encoding.js n=100000 input="1"                   0.67 %             0.55243180
 util/normalize-encoding.js n=100000 input="BASE64"              1.58 %             0.15997022
 util/normalize-encoding.js n=100000 input="BINARY"              0.68 %             0.63394713
 util/normalize-encoding.js n=100000 input="HEX"                -0.04 %             0.97719285
 util/normalize-encoding.js n=100000 input="UCS2"               -1.31 %             0.17303227
 util/normalize-encoding.js n=100000 input="UTF-16LE"           -1.44 %             0.44686975
 util/normalize-encoding.js n=100000 input="UTF-8"               1.21 %             0.52612697
 util/normalize-encoding.js n=100000 input="UTF16LE"            -0.91 %             0.68579514
 util/normalize-encoding.js n=100000 input="UTF8"                0.04 %             0.98501700
 util/normalize-encoding.js n=100000 input="Utf8"               -1.17 %             0.28905432
 util/normalize-encoding.js n=100000 input="[]"                 -0.16 %             0.91167510
 util/normalize-encoding.js n=100000 input="base64"              1.77 %             0.48811676
 util/normalize-encoding.js n=100000 input="binary"              1.15 %             0.57701763
 util/normalize-encoding.js n=100000 input="false"               0.25 %             0.86446726
 util/normalize-encoding.js n=100000 input="foo"                 1.99 %             0.23020110
 util/normalize-encoding.js n=100000 input="group_common"       -1.24 %             0.39849210
 util/normalize-encoding.js n=100000 input="group_misc"          1.13 %             0.38549956
 util/normalize-encoding.js n=100000 input="group_uncommon"     -0.87 %             0.55730142
 util/normalize-encoding.js n=100000 input="group_upper"         0.12 %             0.93350800
 util/normalize-encoding.js n=100000 input="hex"                -0.28 %             0.81607127
 util/normalize-encoding.js n=100000 input="latin1"              0.08 %             0.96807529
 util/normalize-encoding.js n=100000 input="uTf-8"               0.56 %             0.69379026
 util/normalize-encoding.js n=100000 input="ucs2"               -2.05 %             0.24612020
 util/normalize-encoding.js n=100000 input="undefined"          -2.05 %             0.43810156
 util/normalize-encoding.js n=100000 input="utF-8"              -2.56 %             0.06979828
 util/normalize-encoding.js n=100000 input="utf-16le"           -1.58 %             0.21587500
 util/normalize-encoding.js n=100000 input="utf-8"               4.39 %             0.29529870
 util/normalize-encoding.js n=100000 input="utf16le"             0.92 %             0.57924116
 util/normalize-encoding.js n=100000 input="utf8"                2.30 %             0.11971079

Against master:

See benchmark results 
                                                            improvement significant    p.value
 util/normalize-encoding.js n=100000 input=""                    1.89 %             0.51563721
 util/normalize-encoding.js n=100000 input="1"                  -3.95 %             0.07231164
 util/normalize-encoding.js n=100000 input="BASE64"              0.02 %             0.98998222
 util/normalize-encoding.js n=100000 input="BINARY"              0.39 %             0.81495762
 util/normalize-encoding.js n=100000 input="HEX"                 2.35 %             0.12803082
 util/normalize-encoding.js n=100000 input="UCS2"               -1.47 %             0.45288292
 util/normalize-encoding.js n=100000 input="UTF-16LE"            0.06 %             0.96956473
 util/normalize-encoding.js n=100000 input="UTF-8"               0.11 %             0.91848772
 util/normalize-encoding.js n=100000 input="UTF16LE"            -1.04 %             0.50024308
 util/normalize-encoding.js n=100000 input="UTF8"                0.12 %             0.93320841
 util/normalize-encoding.js n=100000 input="Utf8"               -1.20 %             0.57602093
 util/normalize-encoding.js n=100000 input="[]"                  1.85 %             0.09944438
 util/normalize-encoding.js n=100000 input="base64"              1.62 %             0.44022027
 util/normalize-encoding.js n=100000 input="binary"              0.89 %             0.48502009
 util/normalize-encoding.js n=100000 input="false"              -1.37 %             0.57340827
 util/normalize-encoding.js n=100000 input="foo"                -1.14 %             0.54335820
 util/normalize-encoding.js n=100000 input="group_common"        1.69 %             0.30794851
 util/normalize-encoding.js n=100000 input="group_misc"          1.82 %             0.14879427
 util/normalize-encoding.js n=100000 input="group_uncommon"      0.07 %             0.95924675
 util/normalize-encoding.js n=100000 input="group_upper"         1.72 %             0.20659181
 util/normalize-encoding.js n=100000 input="hex"                 3.19 %             0.05784523
 util/normalize-encoding.js n=100000 input="latin1"             -3.32 %             0.08724869
 util/normalize-encoding.js n=100000 input="uTf-8"               0.18 %             0.89152942
 util/normalize-encoding.js n=100000 input="ucs2"                1.32 %             0.54429200
 util/normalize-encoding.js n=100000 input="undefined"          -0.18 %             0.94830130
 util/normalize-encoding.js n=100000 input="utF-8"              -1.96 %             0.40376120
 util/normalize-encoding.js n=100000 input="utf-16le"            1.57 %             0.32425724
 util/normalize-encoding.js n=100000 input="utf-8"              -0.07 %             0.97425237
 util/normalize-encoding.js n=100000 input="utf16le"            -2.09 %             0.26998988
 util/normalize-encoding.js n=100000 input="utf8"                0.97 %             0.70922856

mscdex commented Jan 10, 2017

Copy link
Copy Markdown
Contributor

and increasing n does not help with the significance?

joyeecheung commented Jan 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

While I'm running the benchmarks, just took a look into the asm, doesn't seem to change though(just one more block without generated code).

See control flow graph

New:

Old:

HIR of that extra B38: 
    begin_HIR
      0 0 v339 BlockEntry  type:Tagged pos:2_59 <|@
      0 0 v340 Simulate id=36 type:Tagged pos:2_59 <|@
      0 0 v341 Goto B39 type:Tagged pos:2_59 <|@
    end_HIR

ASM:

0xddce0c5e8ef  comment  (;;; <@436,#339> -------------------- B38 (unreachable/replaced) --------------------)

mscdex commented Jan 10, 2017

Copy link
Copy Markdown
Contributor

@AndreasMadsen any ideas why no significance when comparing with the same binary (especially since it's not performing network requests or similar I/O)?

AndreasMadsen commented Jan 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

any ideas why no significance when comparing with the same binary

I don't understand the question. You are expecting significant results when comparing master against master? I would not expect that, since the performance should be the same.

(especially since it's not performing network requests or similar I/O)?

It is quite easy to get false positives by just running something in the background. Unfortunately it doesn't have to be I/O. This is why nodejs/benchmarking#58 is so important.

joyeecheung added a commit that referenced this pull request Jan 14, 2017
benchmark: use "confidence" in output of compare.R
f18b463 
Use the word "confidence" to indicate the confidence level of
the p value so it's easier to understand.
With this change more stars in the output of compare.R means
higher confidence level (lower significance level).

PR-URL: #10737
Refs: #10439
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
util: improve readability of normalizeEncoding
f329486 
* Improve readability of util.normalizeEncoding
  and add some comments
* Add a benchmark for util.normalizeEncoding
joyeecheung force-pushed the refactor-normalize-encoding branch from 267386d to f329486 Compare January 14, 2017 07:43

joyeecheung commented Jan 14, 2017

Copy link
Copy Markdown
Member Author

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

joyeecheung commented Jan 14, 2017

Copy link
Copy Markdown
Member Author

@mscdex I am going to land this if there are no more concerns.

joyeecheung commented Jan 15, 2017

Copy link
Copy Markdown
Member Author

Landed in 34bf31e

joyeecheung added a commit that referenced this pull request Jan 15, 2017
util: improve readability of normalizeEncoding
34bf31e 
* Improve readability of util.normalizeEncoding
  and add some comments
* Add a benchmark for util.normalizeEncoding

PR-URL: #10439
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
benchmark: use "confidence" in output of compare.R
5fb91ed 
Use the word "confidence" to indicate the confidence level of
the p value so it's easier to understand.
With this change more stars in the output of compare.R means
higher confidence level (lower significance level).

PR-URL: nodejs#10737
Refs: nodejs#10439
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
util: improve readability of normalizeEncoding
5322603 
* Improve readability of util.normalizeEncoding
  and add some comments
* Add a benchmark for util.normalizeEncoding

PR-URL: nodejs#10439
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017
benchmark: use "confidence" in output of compare.R
a8dbdae 
Use the word "confidence" to indicate the confidence level of
the p value so it's easier to understand.
With this change more stars in the output of compare.R means
higher confidence level (lower significance level).

PR-URL: nodejs#10737
Refs: nodejs#10439
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017
util: improve readability of normalizeEncoding
53683e0 
* Improve readability of util.normalizeEncoding
  and add some comments
* Add a benchmark for util.normalizeEncoding

PR-URL: nodejs#10439
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
benchmark: use "confidence" in output of compare.R
178693d 
Use the word "confidence" to indicate the confidence level of
the p value so it's easier to understand.
With this change more stars in the output of compare.R means
higher confidence level (lower significance level).

PR-URL: nodejs#10737
Refs: nodejs#10439
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
util: improve readability of normalizeEncoding
d621f58 
* Improve readability of util.normalizeEncoding
  and add some comments
* Add a benchmark for util.normalizeEncoding

PR-URL: nodejs#10439
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
italoacasas mentioned this pull request Jan 25, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
benchmark: use "confidence" in output of compare.R
ecf72d8 
Use the word "confidence" to indicate the confidence level of
the p value so it's easier to understand.
With this change more stars in the output of compare.R means
higher confidence level (lower significance level).

PR-URL: nodejs#10737
Refs: nodejs#10439
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
util: improve readability of normalizeEncoding
9f6d1f6 
* Improve readability of util.normalizeEncoding
  and add some comments
* Add a benchmark for util.normalizeEncoding

PR-URL: nodejs#10439
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
italoacasas mentioned this pull request Jan 29, 2017
Merged
joyeecheung deleted the refactor-normalize-encoding branch February 19, 2017 17:44
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
util: improve readability of normalizeEncoding
912873b 
* Improve readability of util.normalizeEncoding
  and add some comments
* Add a benchmark for util.normalizeEncoding

PR-URL: #10439
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
util: improve readability of normalizeEncoding
5321300 
* Improve readability of util.normalizeEncoding
  and add some comments
* Add a benchmark for util.normalizeEncoding

PR-URL: #10439
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
MylesBorins mentioned this pull request Mar 9, 2017
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
+1 more reviewer
mscdex mscdex left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

benchmark Issues and PRs related to the benchmark subsystem. util Issues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL