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

buffer: removed unused variable conf from buffer-base64-decode & buffer-base64-encode by troy0820 · Pull Request #10175 · nodejs/node · GitHub

/ node Public

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

Closed
troy0820 wants to merge 4 commits into
nodejs:masterfrom
troy0820:troy0820-code-and-learn
Closed

buffer: removed unused variable conf from buffer-base64-decode & buffer-base64-encode#10175
troy0820 wants to merge 4 commits into
nodejs:masterfrom
troy0820:troy0820-code-and-learn

Conversation

troy0820 commented Dec 7, 2016

Copy link
Copy Markdown
Contributor

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 added the benchmark Issues and PRs related to the benchmark subsystem. label Dec 7, 2016

mscdex commented Dec 8, 2016

Copy link
Copy Markdown
Contributor

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 added the buffer Issues and PRs related to the buffer subsystem. label Dec 8, 2016

Trott commented Dec 8, 2016
edited
Loading

Copy link
Copy Markdown
Member

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 commented Dec 8, 2016

Copy link
Copy Markdown
Contributor

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

troy0820 commented Dec 8, 2016

Copy link
Copy Markdown
Contributor Author

@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 commented Dec 8, 2016

Copy link
Copy Markdown
Contributor

@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 commented Dec 8, 2016

Copy link
Copy Markdown
Member

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 commented Dec 8, 2016
edited
Loading

Copy link
Copy Markdown
Contributor

@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 commented Dec 8, 2016

Copy link
Copy Markdown
Member

@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 commented Dec 8, 2016
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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 commented Dec 8, 2016

Copy link
Copy Markdown
Member

@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 force-pushed the troy0820-code-and-learn branch from bb1f3b1 to a2af2c7 Compare December 8, 2016 21:36

troy0820 commented Dec 8, 2016

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This should be +conf.n.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

troy0820 force-pushed the troy0820-code-and-learn branch 2 times, most recently from ac30244 to 3edffd5 Compare December 9, 2016 15:02

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jasnell commented Dec 23, 2016

Copy link
Copy Markdown
Member

Ping @mscdex

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This should be const n = +conf.n;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This should be const len = +conf.len;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

N here should be len

Copy link
Copy Markdown
Contributor Author

Rebased master and made changes @mscdex

mscdex commented Dec 23, 2016

Copy link
Copy Markdown
Contributor

LGTM

Trott commented Dec 23, 2016

Copy link
Copy Markdown
Member

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

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

Trott commented Dec 23, 2016

Copy link
Copy Markdown
Member

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 commented Dec 23, 2016

Copy link
Copy Markdown
Member

Linter is ✅

Trott force-pushed the troy0820-code-and-learn branch from 8f096db to 08cdc9c Compare December 23, 2016 19:46
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 commented Dec 23, 2016

Copy link
Copy Markdown
Member

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

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 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>
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
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


Back | FazBrowse Home | New Git URL