Skip to content

Conversation

@AndreasMadsen
Copy link
Member

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.

@Fishrock123Fishrock123 added discuss Issues opened for discussions and feedbacks. benchmark Issues and PRs related to the benchmark subsystem. labels Feb 10, 2016
@jasnell
Copy link
Member

+1 ! LGTM

@AndreasMadsen
Copy link
MemberAuthor

@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
Copy link
MemberAuthor

@jasnell@Fishrock123 ping

@jasnell
Copy link
Member

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

@rvagg
Copy link
Member

lgtm, ping @nodejs/benchmarking anyone object here?

@AndreasMadsen
Copy link
MemberAuthor

rebased again because of conflicts with 4bb529d.

@jasnell@rvagg can we merge?

@rvagg
Copy link
Member

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
Copy link
MemberAuthor

@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
Copy link
Member

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
Copy link
MemberAuthor

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 

rvagg pushed a commit that referenced this pull request Feb 26, 2016
PR-URL: #5177 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 26, 2016
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 <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 26, 2016
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 <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 26, 2016
PR-URL: #5177 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
@rvagg
Copy link
Member

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

@rvaggrvagg closed this Feb 26, 2016
@petkaantonov
Copy link
Contributor

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

@rvagg
Copy link
Member

rvagg pushed a commit that referenced this pull request Feb 27, 2016
PR-URL: #5177 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 27, 2016
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 <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 27, 2016
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 <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 27, 2016
PR-URL: #5177 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
PR-URL: #5177 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
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 <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
PR-URL: #5177 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
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 <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Jun 24, 2016
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
PR-URL: #5177 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
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 <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
PR-URL: #5177 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
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 <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
PR-URL: #5177 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
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 <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
PR-URL: #5177 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
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 <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

benchmarkIssues and PRs related to the benchmark subsystem.discussIssues opened for discussions and feedbacks.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@AndreasMadsen@jasnell@rvagg@petkaantonov@MylesBorins@Fishrock123