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

Benchmark: add destructuring object #8680

Closed
wants to merge 1 commit into from
Closed

Conversation

fundon
Copy link
Contributor

@fundon fundon commented Sep 21, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
Affected core subsystem(s)
Description of change

Maybe throse codes should be changed:

FROM

https://github.com/nodejs/node/blob/master/lib/fs.js#L1-L40

TO
// Maintainers, keep in mind that ES1-style octal literals (`0666`) are not
// allowed in strict mode. Use ES6-style octal literals instead (`0o666`).

'use strict';

const { fs: constants } = process.binding('constants');
const util = require('util');
const pathModule = require('path');

const binding = process.binding('fs');
const fs = exports;
const { Buffer } = require('buffer');
const { Stream } = require('stream');
const EventEmitter = require('events');
const { FSReqWrap } = binding;
const { FSEvent } = process.binding('fs_event_wrap');
const { assertEncoding, SyncWriteStream } = require('internal/fs');

Object.defineProperty(exports, 'constants', {
  configurable: false,
  enumerable: true,
  value: constants
});

const { Readable, Writable } = Stream;

const kMinPoolSpace = 128;
const { kMaxLength } = require('buffer');

const {
  O_APPEND = 0,
  O_CREAT = 0,
  O_EXCL = 0,
  O_RDONLY = 0,
  O_RDWR = 0,
  O_SYNC = 0,
  O_TRUNC = 0,
  O_WRONLY = 0
} = constants;

Benchmark Results: ~~~23% improvement~~ higher is better

 λ ~/D/o/node git:(master) 1≡ > ./node benchmark/es/destructuring-object-bench.js
es/destructuring-object-bench.js millions=100 method="normal": 392.4462171885431
es/destructuring-object-bench.js millions=100 method="destructureObject": 300.60502352237313
 λ ~/D/o/node git:(master) 1≡ > ./node benchmark/es/destructuring-object-bench.js
es/destructuring-object-bench.js millions=100 method="normal": 378.884718277464
es/destructuring-object-bench.js millions=100 method="destructureObject": 306.7606096492869
 λ ~/D/o/node git:(master) 1≡ > ./node benchmark/es/destructuring-object-bench.js
es/destructuring-object-bench.js millions=100 method="normal": 385.192791572974
es/destructuring-object-bench.js millions=100 method="destructureObject": 296.1235505392568

@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Sep 21, 2016
var x = o.x;
var y = o.y;
var r = o.r || 2;
assert.strictEqual(x, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

These assert calls are unnecessary and will just slow down the benchmark.

Copy link
Contributor Author

@fundon fundon Sep 21, 2016

Choose a reason for hiding this comment

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

I just follow other benchmark cases. Need to delete them?

Copy link
Contributor Author

@fundon fundon Sep 21, 2016

Choose a reason for hiding this comment

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

Remove all assert and results: ~38% improvement higher is better

 λ ~/D/o/node git:(master) 1≡ 1M > ./node benchmark/es/destructuring-object-bench.js
es/destructuring-object-bench.js millions=100 method="normal": 903.2505158892741
es/destructuring-object-bench.js millions=100 method="destructureObject": 564.4660663547472
 λ ~/D/o/node git:(master) 1≡ 1M > ./node benchmark/es/destructuring-object-bench.js
es/destructuring-object-bench.js millions=100 method="normal": 890.7910802024818
es/destructuring-object-bench.js millions=100 method="destructureObject": 547.1594066200685
 λ ~/D/o/node git:(master) 1≡ 1M > ./node benchmark/es/destructuring-object-bench.js
es/destructuring-object-bench.js millions=100 method="normal": 900.7505972742099
es/destructuring-object-bench.js millions=100 method="destructureObject": 523.6524462787967

@mscdex
Copy link
Contributor

mscdex commented Sep 21, 2016

The benchmark results you've posted show that non-destructuring is faster (higher number, since results are ops/sec.). Even so, having this benchmark might be nice to have anyway.

@fundon
Copy link
Contributor Author

fundon commented Sep 21, 2016

😂 I think it's excuted times. Sorry

@mscdex
Copy link
Contributor

mscdex commented Sep 21, 2016

If you remove the assert calls, the adding of this benchmark LGTM.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

@imyller imyller self-assigned this Sep 23, 2016
@imyller
Copy link
Member

imyller commented Sep 23, 2016

@imyller
Copy link
Member

imyller commented Sep 23, 2016

@fundon Before we can land this, could you please address following linter errors:

destructuring-object-bench.js
  15:9   error  'x' is defined but never used  no-unused-vars
  16:9   error  'y' is defined but never used  no-unused-vars
  17:9   error  'r' is defined but never used  no-unused-vars
  27:11  error  'x' is defined but never used  no-unused-vars
  27:14  error  'y' is defined but never used  no-unused-vars
  27:17  error  'r' is defined but never used  no-unused-vars

@mscdex
Copy link
Contributor

mscdex commented Sep 23, 2016

I think we can add code comments to ignore those particular linter errors?

@imyller
Copy link
Member

imyller commented Sep 24, 2016

I think we can add code comments to ignore those particular linter errors?

I agree.

@fundon You can enclose lines 15-17 and line 27 with:

/* eslint-disable no-unused-vars */
 ...
/* eslint-enable no-unused-vars */

@fundon
Copy link
Contributor Author

fundon commented Sep 24, 2016

Ok, I update it.

@imyller
Copy link
Member

imyller commented Sep 24, 2016

@fundon
Copy link
Contributor Author

fundon commented Sep 24, 2016

Some tests failed. What should i do?

@imyller
Copy link
Member

imyller commented Sep 24, 2016

@fundon CI failures are unrelated. They all seem to be known issues.

I'll start prepping this PR for landing in few hours 👍

@imyller
Copy link
Member

imyller commented Sep 24, 2016

Landing:

  • Three LGTMs
  • No objections
  • Requested changes have been made
  • CI test passed (only known and unrelated CI failures)

imyller pushed a commit to imyller/node that referenced this pull request Sep 24, 2016
PR-URL: nodejs#8680
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
@imyller
Copy link
Member

imyller commented Sep 24, 2016

landed in 7458fbb

Thank you, @fundon

@imyller imyller closed this Sep 24, 2016
@imyller imyller removed their assignment Sep 24, 2016
jasnell pushed a commit that referenced this pull request Sep 29, 2016
PR-URL: #8680
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
geek pushed a commit to geek/node that referenced this pull request Sep 30, 2016
PR-URL: nodejs#8680
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
PR-URL: #8680
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants