GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — assert: typed array deepequal performance fix by claudiorodriguez · Pull Request #4330 · nodejs/node

Conversation

@claudiorodriguez

@claudiorodriguez claudiorodriguez commented Dec 17, 2015

Copy link
Copy Markdown
Contributor

assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

Fixes: #4294

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Fishrock123 Fishrock123 added the assert Issues and PRs related to the assert subsystem. label Dec 17, 2015
@jasnell

jasnell commented Dec 17, 2015

Copy link
Copy Markdown
Member

LGTM

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cjihrig
cjihrig reviewed Dec 17, 2015
View reviewed changes
Comment thread lib/assert.js Outdated

@cjihrig cjihrig Dec 17, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can this logic be pushed down further? At least past the "7.1." check on line 152.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@cjihrig

cjihrig commented Dec 17, 2015

Copy link
Copy Markdown
Contributor

The test should probably have some cases where the two values are not equal.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@cjihrig

cjihrig commented Dec 17, 2015

Copy link
Copy Markdown
Contributor

This seems fine, but I'd like feedback from other collaborators. Are there any edge cases where this change could come back to bite us?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@claudiorodriguez
claudiorodriguez force-pushed the assert-performance-typed-arrays branch 2 times, most recently from 83468ba to 10d83e5 Compare December 17, 2015 17:40
@claudiorodriguez

claudiorodriguez commented Dec 17, 2015

Copy link
Copy Markdown
Contributor Author

@cjihrig Thanks, just pushed the changes, +1 on waiting for feedback

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Trott
Trott reviewed Dec 17, 2015
View reviewed changes
Comment thread lib/assert.js Outdated

@Trott Trott Dec 17, 2015

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

These numbers (7.5) are not arbitrary! They come from the CommonJS standard. Looks like they may already be goobered up, but that's not a reason to make them further-removed from the standard they are intended to refer to.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@claudiorodriguez
claudiorodriguez force-pushed the assert-performance-typed-arrays branch from 10d83e5 to 94bb526 Compare December 17, 2015 19:42
@claudiorodriguez

claudiorodriguez commented Dec 17, 2015

Copy link
Copy Markdown
Contributor Author

@Trott Oops, thanks for catching that, fixed jslint issues.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@claudiorodriguez
claudiorodriguez force-pushed the assert-performance-typed-arrays branch from 94bb526 to 5b14d55 Compare December 17, 2015 19:47
@claudiorodriguez

claudiorodriguez commented Dec 17, 2015

Copy link
Copy Markdown
Contributor Author

@Trott You really learn something new every day, thanks again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Trott

Trott commented Dec 17, 2015

Copy link
Copy Markdown
Member

Is it worth taking a benchmark to confirm the performance improvement? A benchmark that we could run across different operating systems and hardware on the CI server might be useful.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@claudiorodriguez
claudiorodriguez force-pushed the assert-performance-typed-arrays branch from 5b14d55 to 8f6a57c Compare December 17, 2015 21:04
@claudiorodriguez

claudiorodriguez commented Dec 17, 2015

Copy link
Copy Markdown
Contributor Author

@Trott you mean something like what I just pushed? Kinda copied over another benchmark's code. Comparison between assert with the fix and without it shows a massive improvement. (20 vs 0.08)

PS: jslint wasn't happy about the benchmark, in fact it isn't happy about a lot of files in the benchmark folder... I think benchmarks are a tad obsolete in general

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@claudiorodriguez
claudiorodriguez force-pushed the assert-performance-typed-arrays branch from 8f6a57c to f79654a Compare December 17, 2015 21:14
@Trott

Trott commented Dec 17, 2015

Copy link
Copy Markdown
Member

@fansworld-claudio On the benchmark file: Yep, exactly. Thanks! 👍

And yes, the benchmark directory is not included in make jslint so, yeah, I guess it's not surprising that there's lots of stuff in there that isn't compliant with other parts of the code base. (Currently, make jslint lints src, lib, test, and tools/eslint-rules only.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@silverwind

silverwind commented Dec 18, 2015

Copy link
Copy Markdown
Contributor

Can we see some benchmark numbers for other primitve type? There should be a regression, hopefully small.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@claudiorodriguez

claudiorodriguez commented Dec 18, 2015

Copy link
Copy Markdown
Contributor Author

@silverwind Good idea, I'll have it tomorrow. There should only be a very slight impact when asserting inside huge loops.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@claudiorodriguez
assert: typed array deepequal performance fix
a6a874b
assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

Fixes: nodejs#4294
@claudiorodriguez
claudiorodriguez force-pushed the assert-performance-typed-arrays branch from f79654a to a6a874b Compare December 18, 2015 12:52
@claudiorodriguez

claudiorodriguez commented Dec 18, 2015

Copy link
Copy Markdown
Contributor Author

@silverwind added two benchmarks:

big array objects (falling into 7.5):

before PR:

assert\deepequal-prims-and-objs-big-array.js prim=null n=25: 1.14118
assert\deepequal-prims-and-objs-big-array.js prim=undefined n=25: 1.16831
assert\deepequal-prims-and-objs-big-array.js prim=a n=25: 1.21148
assert\deepequal-prims-and-objs-big-array.js prim=1 n=25: 1.22213
assert\deepequal-prims-and-objs-big-array.js prim=true n=25: 1.14505
assert\deepequal-prims-and-objs-big-array.js prim=[object Object] n=25: 1.22749
assert\deepequal-prims-and-objs-big-array.js prim=1,2,3 n=25: 1.23757
assert\deepequal-prims-and-objs-big-array.js prim=1,2,3 n=25: 1.20910

after PR:

assert\deepequal-prims-and-objs-big-array.js prim=null n=25: 1.22064
assert\deepequal-prims-and-objs-big-array.js prim=undefined n=25: 1.20545
assert\deepequal-prims-and-objs-big-array.js prim=a n=25: 1.19943
assert\deepequal-prims-and-objs-big-array.js prim=1 n=25: 1.21435
assert\deepequal-prims-and-objs-big-array.js prim=true n=25: 1.14204
assert\deepequal-prims-and-objs-big-array.js prim=[object Object] n=25: 1.18320
assert\deepequal-prims-and-objs-big-array.js prim=1,2,3 n=25: 1.19306
assert\deepequal-prims-and-objs-big-array.js prim=1,2,3 n=25: 1.21798

The fluctuations seem to be within random variance (in my computer's case), so there doesn't seem to be a real change here.


big loops (forcing into 7.5 with array object):

before PR:

assert\deepequal-prims-and-objs-big-loop.js prim=null n=100000: 31422.79244
assert\deepequal-prims-and-objs-big-loop.js prim=undefined n=100000: 28421.44023
assert\deepequal-prims-and-objs-big-loop.js prim=a n=100000: 30207.53243
assert\deepequal-prims-and-objs-big-loop.js prim=1 n=100000: 29245.98661
assert\deepequal-prims-and-objs-big-loop.js prim=true n=100000: 25392.93322
assert\deepequal-prims-and-objs-big-loop.js prim=[object Object] n=100000: 29751.57255
assert\deepequal-prims-and-objs-big-loop.js prim=1,2,3 n=100000: 29663.82391
assert\deepequal-prims-and-objs-big-loop.js prim=1,2,3 n=100000: 32222.55574

after PR:

assert\deepequal-prims-and-objs-big-loop.js prim=null n=100000: 28553.61637
assert\deepequal-prims-and-objs-big-loop.js prim=undefined n=100000: 30250.17099
assert\deepequal-prims-and-objs-big-loop.js prim=a n=100000: 31414.15124
assert\deepequal-prims-and-objs-big-loop.js prim=1 n=100000: 31509.91023
assert\deepequal-prims-and-objs-big-loop.js prim=true n=100000: 31548.13352
assert\deepequal-prims-and-objs-big-loop.js prim=[object Object] n=100000: 31572.05442
assert\deepequal-prims-and-objs-big-loop.js prim=1,2,3 n=100000: 31233.08257
assert\deepequal-prims-and-objs-big-loop.js prim=1,2,3 n=100000: 31618.14617

Also seems to be within random variance, after running it a few times... at least running it on my computer.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@silverwind

silverwind commented Dec 18, 2015

Copy link
Copy Markdown
Contributor

Yeah, looks like v8 is optimizing the extra if branch pretty well and your results look to be just fluctuations. Thanks for adding these benchmarks 👍

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Dec 21, 2015

Copy link
Copy Markdown
Member

LGTM

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

1 similar comment
@silverwind

silverwind commented Dec 21, 2015

Copy link
Copy Markdown
Contributor

LGTM

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@cjihrig

cjihrig commented Dec 21, 2015

Copy link
Copy Markdown
Contributor

LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/1037/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

silverwind pushed a commit that referenced this pull request Dec 21, 2015
@claudiorodriguez @silverwind
assert: typed array deepequal performance fix
6378622
assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

PR-URL: #4330
Fixes: #4294
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@silverwind

silverwind commented Dec 21, 2015

Copy link
Copy Markdown
Contributor

Thanks! Landed in 6378622.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@silverwind silverwind closed this Dec 21, 2015
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Dec 22, 2015
@claudiorodriguez @Fishrock123
assert: typed array deepequal performance fix
dd935c2
assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

PR-URL: nodejs#4330
Fixes: nodejs#4294
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell jasnell added the lts-watch-v4.x label Dec 24, 2015
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
@claudiorodriguez @Fishrock123
assert: typed array deepequal performance fix
d265fc8
assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

PR-URL: nodejs#4330
Fixes: nodejs#4294
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
@Fishrock123
2016-01-06, Version 5.4.0 (Stable)
af591d4
* http:
  - A new status code was added: 451 - "Unavailable For Legal Reasons" (Max Barinov) nodejs#4377
  - Idle sockets that have been kept alive now handle errors (José F. Romaniello) nodejs#4482
* This release also includes several minor performance improvements:
  - assert: deepEqual is now speedier when comparing TypedArrays (Claudio Rodriguez) nodejs#4330
  - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622
  - node: Improved accessor perf of process.env (Trevor Norris) nodejs#3780
  - node: Improved performance of process.hrtime() (Trevor Norris) nodejs#3780, (Evan Lucas) nodejs#4484
  - node: Improved GetActiveHandles performance (Trevor Norris) nodejs#3780
  - util: Use faster iteration in util.format() (Jackson Tian) nodejs#3964

PR-URL: nodejs#4547
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jan 11, 2016
@Fishrock123
2016-01-06, Version 5.4.0 (Stable)
5f0abd6
* http:
  - A new status code was added: 451 - "Unavailable For Legal Reasons"
(Max Barinov) nodejs#4377
  - Idle sockets that have been kept alive now handle errors (José F.
Romaniello) nodejs#4482
* This release also includes several minor performance improvements:
  - assert: deepEqual is now speedier when comparing TypedArrays
(Claudio Rodriguez) nodejs#4330
  - lib: Use arrow functions instead of bind where possible (Minwoo
Jung) nodejs#3622
  - node: Improved accessor perf of process.env (Trevor Norris)
nodejs#3780
  - node: Improved performance of process.hrtime() (Trevor Norris)
nodejs#3780, (Evan Lucas)
nodejs#4484
  - node: Improved GetActiveHandles performance (Trevor Norris)
nodejs#3780
  - util: Use faster iteration in util.format() (Jackson Tian)
nodejs#3964

Refs: nodejs#4547
PR-URL: nodejs#4623
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 13, 2016
MylesBorins pushed a commit that referenced this pull request Jan 13, 2016
@claudiorodriguez Myles Borins
assert: typed array deepequal performance fix
a22b389
assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

PR-URL: #4330
Fixes: #4294
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
@claudiorodriguez Myles Borins
assert: typed array deepequal performance fix
901172a
assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

PR-URL: #4330
Fixes: #4294
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
V4.2.5 proposal #4768
Merged
@addaleax addaleax mentioned this pull request Mar 26, 2016
assert: Check typed array view type in deepEqual #5910
Closed
3 tasks done
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@claudiorodriguez Michael Scovetta
assert: typed array deepequal performance fix
84e3a08
assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

PR-URL: nodejs#4330
Fixes: nodejs#4294
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@Fishrock123 Michael Scovetta
2016-01-06, Version 5.4.0 (Stable)
3991aa8
* http:
  - A new status code was added: 451 - "Unavailable For Legal Reasons"
(Max Barinov) nodejs#4377
  - Idle sockets that have been kept alive now handle errors (José F.
Romaniello) nodejs#4482
* This release also includes several minor performance improvements:
  - assert: deepEqual is now speedier when comparing TypedArrays
(Claudio Rodriguez) nodejs#4330
  - lib: Use arrow functions instead of bind where possible (Minwoo
Jung) nodejs#3622
  - node: Improved accessor perf of process.env (Trevor Norris)
nodejs#3780
  - node: Improved performance of process.hrtime() (Trevor Norris)
nodejs#3780, (Evan Lucas)
nodejs#4484
  - node: Improved GetActiveHandles performance (Trevor Norris)
nodejs#3780
  - util: Use faster iteration in util.format() (Jackson Tian)
nodejs#3964

Refs: nodejs#4547
PR-URL: nodejs#4623
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@addaleax addaleax mentioned this pull request Apr 11, 2016
assert: Check typed array view type in deepEqual (backport to 4.x) #6147
Closed
3 tasks done
@addaleax addaleax mentioned this pull request Jan 21, 2017
assert: enforce type check in deepStrictEqual #10282
Closed
3 tasks done
@leonardoadame leonardoadame mentioned this pull request Dec 1, 2023
[Snyk] Fix for 1 vulnerabilities leonardoadame/node#377
Open
@aliscco aliscco mentioned this pull request Dec 2, 2023
[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#1015
Open
@leonardoadame leonardoadame mentioned this pull request Feb 11, 2024
[Snyk] Security upgrade webpack-dev-server from 3.11.3 to 4.8.0 leonardoadame/node#646
Open
@aliscco aliscco mentioned this pull request Feb 12, 2024
[Snyk] Security upgrade webpack-dev-server from 3.11.3 to 4.8.0 aliscco/alisco-node#1250
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

assert Issues and PRs related to the assert subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@claudiorodriguez @jasnell @cjihrig @Trott @silverwind @mcollina @MylesBorins @Fishrock123
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.