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

buffer: removed unused variable conf from buffer-base64-decode & buffer-base64-encode #10175

Closed
wants to merge 4 commits into from

Conversation

troy0820
Copy link
Contributor

@troy0820 troy0820 commented Dec 7, 2016

Removed unused variable conf from buffer-base64-decode.js and buffer-based64-encode.js

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

Buffer

Description of change

Deleted variable that wasn't used which was conf

@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Dec 7, 2016
@mscdex
Copy link
Contributor

mscdex commented Dec 8, 2016

I'm not sure it's worth removing. Benchmarks like this probably should have defined parameters containing defaults of at least the currently used values.

@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Dec 8, 2016
@Trott
Copy link
Member

Trott commented Dec 8, 2016

I'm not sure it's worth removing. Benchmarks like this probably should have defined parameters containing defaults of at least the currently used values.

@mscdex Does this mean you would endorse (or at least not object to) a change from this:

var bench = common.createBenchmark(main, {});

function main(conf) {
  var N = 64 * 1024 * 1024;
  var b = Buffer.allocUnsafe(N);
  var s = '';
  var i;
  for (i = 0; i < 256; ++i) s += String.fromCharCode(i);
  for (i = 0; i < N; i += 256) b.write(s, i, 256, 'ascii');
  bench.start();
  for (i = 0; i < 32; ++i) b.toString('base64');
  bench.end(64);
}

...to something more like this?:

var bench = common.createBenchmark(main, {
  N: [64 * 1024 * 1024],
});

function main(conf) {
  var N = +conf.N;
  var b = Buffer.allocUnsafe(N);
  var s = '';
  var i;
  for (i = 0; i < 256; ++i) s += String.fromCharCode(i);
  for (i = 0; i < N; i += 256) b.write(s, i, 256, 'ascii');
  bench.start();
  for (i = 0; i < 32; ++i) b.toString('base64');
  bench.end(64);
}

@mscdex
Copy link
Contributor

mscdex commented Dec 8, 2016

@Trott Pretty much, although the iteration count might be configurable as well like with most other benchmarks.

@troy0820
Copy link
Contributor Author

troy0820 commented Dec 8, 2016

@Trott @mscdex Do you want me to change the commit to the above snippet? I can make the changes and push the commit up to this branch.

@mscdex
Copy link
Contributor

mscdex commented Dec 8, 2016

@troy0820 I think it would be better, yes. Just make sure to also add a parameter for the number of iterations (usually lowercase n).

@Trott
Copy link
Member

Trott commented Dec 8, 2016

number of iterations

@mscdex That would be the number of iterations in the loop between bench.start() and bench.end(), right? So, in the above example, it's 32? Also, would that value be passed to bench.end()? In the example above, 64 is hard-coded as what is sent to bench.end(), so...
¯\(ツ)

@mscdex
Copy link
Contributor

mscdex commented Dec 8, 2016

@Trott Correct. I don't know why 64 was being passed in the encode benchmark, it should be the loop count which is 32.

@Trott
Copy link
Member

Trott commented Dec 8, 2016

@troy0820 You up for making those changes as described above and trying to ascertain what the similar changes might be for the other file? (I'd guess just the iterations and nothing else in that file, but take a look and judge for yourself.)

@troy0820
Copy link
Contributor Author

troy0820 commented Dec 8, 2016

@Trott Yeah I can make those changes. It sounds like it's the above snippet along with the only change being the 32 in the bench.end(32) function and adding then param to the main function.
Only question is do you want 32 to be in the bench.end() function or n?
I'll update the PR 👍

@Trott
Copy link
Member

Trott commented Dec 8, 2016

@troy0820 You'll want to add an n variable to the conf for both files and set it to 32. Then you'll want to assign it to a variable (like I did in the snippet with N) and use that variable in the loop and in bench.end(). Hit me up on IRC if this isn't clear, or just make a go at it and we'll iterate if it's not quite right.

@troy0820
Copy link
Contributor Author

troy0820 commented Dec 8, 2016

Per discussion, adding an array to the createBenchmark object will allow the default buffer size to be included with the object and iteration in the loop. @mscdex @Trott

@@ -1,16 +1,20 @@
'use strict';
var common = require('../common.js');

var bench = common.createBenchmark(main, {});
const bench = common.createBenchmark(main, {
N: [64 * 1024 * 1024],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to give this a better name to avoid confusion. I think other benchmarks use names like len, which I would be fine with.


function main(conf) {
const n = conf.n;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be +conf.n.


function main(conf) {
var N = 64 * 1024 * 1024;
var n = conf.n;
var N = conf.N;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly for these, the values should be prefixed with + to ensure they are numbers.

@@ -3,15 +3,15 @@ var common = require('../common.js');

const bench = common.createBenchmark(main, {
N: [64 * 1024 * 1024],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant this parameter should be called len. n should stay the same.

@troy0820 troy0820 force-pushed the troy0820-code-and-learn branch 2 times, most recently from ac30244 to 3edffd5 Compare December 9, 2016 15:02
for (i = 0; i < 32; ++i) b.toString('base64');
bench.end(64);
for (i = 0; i < n; ++i) b.toString('base64');
bench.end(n);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the change from 64 to 32 in the bench.end(n) here intentional?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind... just spotted the other comment about it :-)

@jasnell
Copy link
Member

jasnell commented Dec 23, 2016

Ping @mscdex

var b = Buffer.allocUnsafe(N);
var s = '';
var i;
const n = +conf.len;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be const n = +conf.n;

var s = '';
var i;
const n = +conf.len;
const N = +conf.N;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be const len = +conf.len;

var i;
const n = +conf.len;
const N = +conf.N;
const b = Buffer.allocUnsafe(N);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be const b = Buffer.allocUnsafe(len);

const N = +conf.N;
const b = Buffer.allocUnsafe(N);
let s = '';
let i;
for (i = 0; i < 256; ++i) s += String.fromCharCode(i);
for (i = 0; i < N; i += 256) b.write(s, i, 256, 'ascii');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

N here should be len

@troy0820
Copy link
Contributor Author

Rebased master and made changes @mscdex

@mscdex
Copy link
Contributor

mscdex commented Dec 23, 2016

LGTM

@Trott
Copy link
Member

Trott commented Dec 23, 2016

Only relevant CI job for this is the linter, so here it is:

CI: https://ci.nodejs.org/job/node-test-linter/6010/

@Trott
Copy link
Member

Trott commented Dec 23, 2016

Hmmm, the linter failed, but that might have been because it needed a rebase, which I just did. Let's try again...

CI: https://ci.nodejs.org/job/node-test-linter/6014/

@Trott
Copy link
Member

Trott commented Dec 23, 2016

Linter is ✅

Trott pushed a commit to Trott/io.js that referenced this pull request Dec 23, 2016
Add configuration object createBenchmark object for buffer size &
iteration in buffer-base64-encode & buffer-base64-decode.js.

PR-URL: nodejs#10175
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@Trott
Copy link
Member

Trott commented Dec 23, 2016

Landed in 7021e6b.
Thanks for the contribution, @troy0820! 🎉

@Trott Trott closed this Dec 23, 2016
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
Add configuration object createBenchmark object for buffer size &
iteration in buffer-base64-encode & buffer-base64-decode.js.

PR-URL: #10175
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
Add configuration object createBenchmark object for buffer size &
iteration in buffer-base64-encode & buffer-base64-decode.js.

PR-URL: #10175
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
Add configuration object createBenchmark object for buffer size &
iteration in buffer-base64-encode & buffer-base64-decode.js.

PR-URL: #10175
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
Add configuration object createBenchmark object for buffer size &
iteration in buffer-base64-encode & buffer-base64-decode.js.

PR-URL: #10175
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
Add configuration object createBenchmark object for buffer size &
iteration in buffer-base64-encode & buffer-base64-decode.js.

PR-URL: #10175
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants