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

Comparing arrays with assert seems to take polynomial time. #12842

Closed
akdor1154 opened this issue May 5, 2017 · 3 comments
Closed

Comparing arrays with assert seems to take polynomial time. #12842

akdor1154 opened this issue May 5, 2017 · 3 comments
Labels
assert Issues and PRs related to the assert subsystem. performance Issues and PRs related to the performance of Node.js.

Comments

@akdor1154
Copy link

akdor1154 commented May 5, 2017

  • Version: 6.10.3 x64
  • Platform: OS X 12.4

Comparing two arrays with assert.deepEqual or .deepStrictEqual seems to take a polynomial amount of time relative to the array size.

Quick code:

function longTest(n) {
		const bigSource = Array.apply(null, Array(n));
		const a1 = bigSource.map((n) => ({ yarp: 'yarp', nope: {yarp: '123', a: [1,2,3]} }));
		const a2 = bigSource.map((n) => ({ yarp: 'yarp', nope: {yarp: '123', a: [1,2,3]} }));
		//a2[a2.length - 1].nope = 'nope';
		const tStart = Date.now();
		assert.deepEqual(a2, a1);
		const tEnd = Date.now();
		const dt = tEnd - tStart;
		return dt;
}

let n = 1;
for (let i = 0; i<=40; i++) {
	n = Math.round(n*1.5);
	const dt = longTest(n);
	console.log(`${n}, ${dt}`)
}

Returns a list of (n),(time) pairs.
On my machine, the output of this is

2, 2
3, 0
5, 0
8, 0
12, 1
18, 1
27, 6
41, 5
62, 1
93, 1
140, 2
210, 3
315, 5
473, 7
710, 11
1065, 21
1598, 40
2397, 70
3596, 135
5394, 270
8091, 581
12137, 1184
18206, 2590
27309, 5679
40964, 12539
61446, 28906
92169, 63617

Graphed, this looks like
linear

Or to illustrate what I think I'm seeing, we can plot on a log-log scale graph:
loglog

Just to be explicit, I would expect this operation to take O(n) time.

I'm quite possibly doing something very silly, so apologies in advance if I'm unknowingly spluttering over my own mistakes.

@akdor1154 akdor1154 changed the title Comparing arrays with assert seems to take O(exp(n)) time. Comparing arrays with assert seems to take polynomial time. May 5, 2017
@refack
Copy link
Contributor

refack commented May 5, 2017

tl;dr: Yes
The current algorithm does not optimize for Arrays so it recurses into each element, I'm not sure it's O(exp(n)) probably O(n**m) where m is the depth of your elements.

In OSS as in OSS, you are welcome to submit an improvement. If you are interested a good place to start is https://github.com/nodejs/node/blob/master/lib/assert.js#L392

@refack
Copy link
Contributor

refack commented May 5, 2017

@mscdex mscdex added assert Issues and PRs related to the assert subsystem. performance Issues and PRs related to the performance of Node.js. labels May 5, 2017
@addaleax
Copy link
Member

addaleax commented May 5, 2017

It should be O(n²) right now, I think – it’s iterating over the array in a linear fashion, but currently does an .indexOf over all objects it has already seen to check whether possible circular references match.

This is fixable (to O(n log n)), though: #12849

addaleax added a commit to addaleax/node that referenced this issue May 7, 2017
Use a Map instead of an array for checking previously found
cyclic references.

This reduces complexity for an array-of-objects case from
O(n²) to O(n·log n).

Fixes: nodejs#12842
PR-URL: nodejs#12849
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this issue May 19, 2017
Use a Map instead of an array for checking previously found
cyclic references.

This reduces complexity for an array-of-objects case from
O(n²) to O(n·log n).

Fixes: nodejs#12842
PR-URL: nodejs#12849
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants