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

Proposal: fast path for assert.deepEqual with typed arrays #4294

Closed
LinusU opened this issue Dec 15, 2015 · 2 comments
Closed

Proposal: fast path for assert.deepEqual with typed arrays #4294

LinusU opened this issue Dec 15, 2015 · 2 comments
Labels
assert Issues and PRs related to the assert subsystem.

Comments

@LinusU
Copy link
Contributor

LinusU commented Dec 15, 2015

assert.deepEqual is very slow when given large typed arrays. This is however easily worked around by wrapping them in buffers instead. It would be awesome if Node.js could use the same performant code to compare Typed Arrays as it uses for buffers.

This is my current local workaround:

function assertTypedArrayEquals (actual, expected, message) {
  const actualBuffer = new Buffer(actual)
  const expectedBuffer = new Buffer(expected)

  if (actualBuffer.equals(expectedBuffer)) return

  throw new assert.AssertionError({
    message: message,
    actual: actual,
    expected: expected,
    operator: 'deepEqual',
    stackStartFunction: assertTypedArrayEquals
  })
}

This brings the test down to a few milliseconds from over one second.

@mscdex mscdex added the assert Issues and PRs related to the assert subsystem. label Dec 15, 2015
@silverwind silverwind added good first issue Issues that are suitable for first-time contributors. and removed good first issue Issues that are suitable for first-time contributors. labels Dec 16, 2015
@silverwind
Copy link
Contributor

@LinusU can you quantify how much faster this is? Also, it might be interesting how such an optimization will affect other type's assertions.

@LinusU
Copy link
Contributor Author

LinusU commented Dec 16, 2015

Test case:

var assert = require('assert')

var a = new Uint8Array(1e6)
var b = new Uint8Array(1e6)

console.time('typed array')
assert.deepEqual(a, b)
console.timeEnd('typed array')

console.time('buffer')
assert.deepEqual(new Buffer(a), new Buffer(b))
console.timeEnd('buffer')

Result:

typed array: 3216.043ms
buffer: 16.126ms

About 200 times faster for an array of 1,000,000 bytes.

claudiorodriguez added a commit to claudiorodriguez/node that referenced this issue Dec 18, 2015
assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

Fixes: nodejs#4294
Fishrock123 pushed a commit to Fishrock123/node that referenced this issue Dec 22, 2015
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 pushed a commit to Fishrock123/node that referenced this issue Jan 6, 2016
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Jan 13, 2016
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Jan 19, 2016
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this issue Apr 2, 2016
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants