Benchmark cleanup #5177

AndreasMadsen · 2016-02-10T15:59:26Z

I'm working on a big refactor of the benchmark suite. While I'm finishing that, I would like to get these less controversial changes merged.

benchmark: move misc to categorized directories
I'm not sure why these tests where in misc, they where quite easy to categorize.

benchmark: merge url.js with url-resolve.js
url.js was broken since it didn't use the common.js runner. This fixes
that issue by merging it with url-resolve.js, which also benchmarks
url.resolve.

~~benchmark: move dgram to its own directory~~
~~Again, trying to give the directories a bit more meaning.~~

benchmark: fix configuation parameters
The benchmark runner spawns new processes for each configuration. The
specific configuration is transfered by process.argv. This means that
the values have to be parsed. As of right now only numbers and strings
are parsed correctly. However other values such as objects where used.

This fixes the benchmarks that used non-string/number values and
prevents future issues by asserting the type.

jasnell · 2016-02-10T17:01:39Z

+1 ! LGTM

AndreasMadsen · 2016-02-14T11:41:52Z

@jasnell can we merge this. I just had to rebase because it conflicted with #4374. (I just removed the "move dgram" commit).

@Fishrock123 you added a "discuss" label, I assume that means something should be discussed. Can you elaborate on that.

AndreasMadsen · 2016-02-19T15:25:29Z

@jasnell @Fishrock123 ping

jasnell · 2016-02-19T22:52:54Z

I'd like to get another LGTM at least. @nodejs/ctc

rvagg · 2016-02-23T03:33:35Z

lgtm, ping @nodejs/benchmarking anyone object here?

AndreasMadsen · 2016-02-25T21:18:11Z

rebased again because of conflicts with 4bb529d.

@jasnell @rvagg can we merge?

rvagg · 2016-02-26T07:21:32Z

doesn't merge cleanly, can you rebase again?

url.js was broken since it didn't use the common.js runner. This fixes that issue by merging it with url-resolve.js, which also benchmarks url.resolve.

The benchmark runner spawns new processes for each configuration. The specific configuration is transfered by process.argv. This means that the values have to be parsed. As of right now only numbers and strings are parsed correctly. However other values such as objects where used. This fixes the benchmarks that used non-string/number values and prevents future issues by asserting the type.

AndreasMadsen · 2016-02-26T08:28:50Z

@rvagg that is odd. It merges cleanly on my screen and the only commits since the rebase where doc related. In any case I have rebased again.

rvagg · 2016-02-26T09:00:11Z

Applying: benchmark: move misc to categorized directories
error: patch failed: benchmark/misc/string-decoder.js:1
error: benchmark/misc/string-decoder.js: patch does not apply
Patch failed at 0001 benchmark: move misc to categorized directories
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: git am exited with code: 128
error: patch failed: benchmark/misc/string-decoder.js:1
error: benchmark/misc/string-decoder.js: patch does not apply

still getting this on applying this as a patch via git am

AndreasMadsen · 2016-02-26T09:19:43Z

I don't know what to say. I just ran this:

$ git clone https://github.com/nodejs/node.git node-test
Cloning into 'node-test'...
remote: Counting objects: 206726, done.
remote: Compressing objects: 100% (19/19), done.
remote: Total 206726 (delta 2), reused 0 (delta 0), pack-reused 206707
Receiving objects: 100% (206726/206726), 179.59 MiB | 2.82 MiB/s, done.
Resolving deltas: 100% (150338/150338), done.
Checking connectivity... done.
Checking out files: 100% (17861/17861), done.

$ cd node-test

$ curl https://patch-diff.githubusercontent.com/raw/nodejs/node/pull/5177.patch | git am
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 41325    0 41325    0     0  29116      0 --:--:--  0:00:01 --:--:-- 29102
Applying: benchmark: move misc to categorized directories
Applying: benchmark: merge url.js with url-resolve.js
Applying: benchmark: fix configuation parameters
Applying: benchmark: move string-decoder to its own category

PR-URL: #5177 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

url.js was broken since it didn't use the common.js runner. This fixes that issue by merging it with url-resolve.js, which also benchmarks url.resolve. PR-URL: #5177 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

The benchmark runner spawns new processes for each configuration. The specific configuration is transfered by process.argv. This means that the values have to be parsed. As of right now only numbers and strings are parsed correctly. However other values such as objects where used. This fixes the benchmarks that used non-string/number values and prevents future issues by asserting the type. PR-URL: #5177 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

PR-URL: #5177 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

rvagg · 2016-02-26T09:30:04Z

Yeah, you're right .. plain curl|git am works, I'm using https://github.com/petkaantonov/apply-pr and as far as I know it does exactly that yet it borks. @petkaantonov have you ever run into a problem where apply-pr won't work but raw curl|git am does?

Merged as:

2426b3d benchmark: move string-decoder to its own category
15720fa benchmark: fix configuation parameters
f6c505d benchmark: merge url.js with url-resolve.js
d9079ab benchmark: move misc to categorized directories

Thanks @AndreasMadsen

petkaantonov · 2016-02-26T10:07:45Z

@rvagg No, I can look into it if you can give the commit id that you tried to apply it against

rvagg · 2016-02-26T10:11:29Z

@petkaantonov 1d7c370