test: test bottom-up merge sort in URLSearchParams #11399

watilde · 2017-02-15T12:35:52Z

Following up for this commit: 02d1e32 to increase the reduced coverage. The bottom-up iterative stable merge sort is called only when the length of provided value is larger than 100. Added a test for it.

This increases the coverage of internal/url.js,

https://coverage.nodejs.org/coverage-e4e7cd5bd202084b/root/internal/url.js.html

and the following lines will be covered.

node/lib/internal/url.js

Lines 701 to 727 in e4e7cd5

    
           function merge(out, start, mid, end, lBuffer, rBuffer) { 
        
             const sizeLeft = mid - start; 
        
             const sizeRight = end - mid; 
        
             var l, r, o; 
        
             for (l = 0; l < sizeLeft; l++) 
        
               lBuffer[l] = out[start + l]; 
        
             for (r = 0; r < sizeRight; r++) 
        
               rBuffer[r] = out[mid + r]; 
        
             l = 0; 
        
             r = 0; 
        
             o = start; 
        
             while (l < sizeLeft && r < sizeRight) { 
        
               if (lBuffer[l] <= rBuffer[r]) { 
        
                 out[o++] = lBuffer[l++]; 
        
                 out[o++] = lBuffer[l++]; 
        
               } else { 
        
                 out[o++] = rBuffer[r++]; 
        
                 out[o++] = rBuffer[r++]; 
        
               } 
        
             } 
        
             while (l < sizeLeft) 
        
               out[o++] = lBuffer[l++]; 
        
             while (r < sizeRight) 
        
               out[o++] = rBuffer[r++]; 
        
           }

node/lib/internal/url.js

Lines 886 to 900 in e4e7cd5

    
           } else { 
        
             // Bottom-up iterative stable merge sort 
        
             const lBuffer = new Array(len); 
        
             const rBuffer = new Array(len); 
        
             for (var step = 2; step < len; step *= 2) { 
        
               for (var start = 0; start < len - 2; start += 2 * step) { 
        
                 var mid = start + step; 
        
                 var end = mid + step; 
        
                 end = end < len ? end : len; 
        
                 if (mid > end) 
        
                   continue; 
        
                 merge(a, start, mid, end, lBuffer, rBuffer); 
        
               } 
        
             } 
        
           }

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

joyeecheung · 2017-02-15T16:01:45Z

test/parallel/test-whatwg-url-searchparams-sort.js

@@ -82,3 +82,34 @@ const { test, assert_array_equals } = common.WPT;
    }
  }, 'URL parse and sort: ' + val.input);
 });
+
+// Test bottom-up iterative stable merge sort
+const params = {input: '', output: []};


If I am reading correctly the test part below just duplicates the code above? Then we can probably just construct the params first and then put it in the array literal that's being forEached above?

That's nice. I've updated it :)

joyeecheung · 2017-02-15T19:04:43Z

test/parallel/test-whatwg-url-searchparams-sort.js

+const tests = [{input: '', output: []}];
+for (let i = 0; 50 > i; i++) {
+  tests[0].input += 'a=b&';
+  tests[0].output.push(['a', 'b']);


Hmm..so looks like this doesn't actually catch a mistake in the implementation because we are sorting identical pairs...maybe we can generate the keys on the fly, pseudo-shuffle them, then compared to another original copy to make sure they are sorted properly?

const tests = [{input: '', output: []}]; const pairs = []; for (let i = 10; i < 60; i++) { pairs.push([`a${i}`, 'b']); tests[0].output.push([`a${i}`, 'b']); } tests[0].input = pairs.sort(() => Math.random() > 0.5).map((pair) => pair.join('=')).join('&');

That's right indeed. To use the random list to sort them actually, I've updated it with your comment. Thanks!

TimothyGu

LGTM.

TimothyGu · 2017-02-16T02:44:42Z

test/parallel/test-whatwg-url-searchparams-sort.js

+// Test bottom-up iterative stable merge sort
+const tests = [{input: '', output: []}];
+const pairs = [];
+for (let i = 10; i < 60; i++) {


nit: I'd be more comfortable if there are more than just the threshold number of elements in pairs

It sounds good to me as well. I will replace 60 with 100 later :)

Ok, I've updated.

TimothyGu · 2017-02-16T02:59:02Z

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

The bottom-up iterative stable merge sort is called only when the length of provided value is larger than 100. Added a test for it.

TimothyGu · 2017-02-16T15:49:02Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/6442/

jasnell · 2017-02-16T19:28:36Z

CI failures are unrelated.

The bottom-up iterative stable merge sort is called only when the length of provided value is larger than 100. Added a test for it. PR-URL: #11399 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

jasnell · 2017-02-19T20:45:54Z

Landed in 1b31ca6

The bottom-up iterative stable merge sort is called only when the length of provided value is larger than 100. Added a test for it. PR-URL: nodejs#11399 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 15, 2017

mscdex added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Feb 15, 2017

joyeecheung reviewed Feb 15, 2017

View reviewed changes

watilde force-pushed the test-internal-url-merge branch 2 times, most recently from e653c27 to cca745a Compare February 15, 2017 18:08

jasnell approved these changes Feb 15, 2017

View reviewed changes

joyeecheung reviewed Feb 15, 2017

View reviewed changes

watilde force-pushed the test-internal-url-merge branch from cca745a to 461fea7 Compare February 15, 2017 23:32

joyeecheung approved these changes Feb 16, 2017

View reviewed changes

TimothyGu approved these changes Feb 16, 2017

View reviewed changes

test: test bottom-up merge sort in URLSearchParams

8176dc3

The bottom-up iterative stable merge sort is called only when the length of provided value is larger than 100. Added a test for it.

watilde force-pushed the test-internal-url-merge branch from 461fea7 to 8176dc3 Compare February 16, 2017 13:03

targos approved these changes Feb 18, 2017

View reviewed changes

jasnell closed this Feb 19, 2017

watilde deleted the test-internal-url-merge branch February 20, 2017 00:33

italoacasas mentioned this pull request Feb 25, 2017

7.7.0 Proposal #11553

Merged

jasnell added dont-land-on-v4.x labels Mar 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: test bottom-up merge sort in URLSearchParams #11399

test: test bottom-up merge sort in URLSearchParams #11399

watilde commented Feb 15, 2017

joyeecheung Feb 15, 2017 •

edited

Loading

watilde Feb 15, 2017

joyeecheung Feb 15, 2017

watilde Feb 15, 2017

TimothyGu left a comment

TimothyGu Feb 16, 2017

watilde Feb 16, 2017

watilde Feb 16, 2017

TimothyGu commented Feb 16, 2017

TimothyGu commented Feb 16, 2017

jasnell commented Feb 16, 2017

jasnell commented Feb 19, 2017

	function merge(out, start, mid, end, lBuffer, rBuffer) {
	const sizeLeft = mid - start;
	const sizeRight = end - mid;
	var l, r, o;

	for (l = 0; l < sizeLeft; l++)
	lBuffer[l] = out[start + l];
	for (r = 0; r < sizeRight; r++)
	rBuffer[r] = out[mid + r];

	l = 0;
	r = 0;
	o = start;
	while (l < sizeLeft && r < sizeRight) {
	if (lBuffer[l] <= rBuffer[r]) {
	out[o++] = lBuffer[l++];
	out[o++] = lBuffer[l++];
	} else {
	out[o++] = rBuffer[r++];
	out[o++] = rBuffer[r++];
	}
	}
	while (l < sizeLeft)
	out[o++] = lBuffer[l++];
	while (r < sizeRight)
	out[o++] = rBuffer[r++];
	}

	} else {
	// Bottom-up iterative stable merge sort
	const lBuffer = new Array(len);
	const rBuffer = new Array(len);
	for (var step = 2; step < len; step *= 2) {
	for (var start = 0; start < len - 2; start += 2 * step) {
	var mid = start + step;
	var end = mid + step;
	end = end < len ? end : len;
	if (mid > end)
	continue;
	merge(a, start, mid, end, lBuffer, rBuffer);
	}
	}
	}

test: test bottom-up merge sort in URLSearchParams #11399

test: test bottom-up merge sort in URLSearchParams #11399

Conversation

watilde commented Feb 15, 2017

Checklist

Affected core subsystem(s)

joyeecheung Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

watilde Feb 15, 2017

Choose a reason for hiding this comment

joyeecheung Feb 15, 2017

Choose a reason for hiding this comment

watilde Feb 15, 2017

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu Feb 16, 2017

Choose a reason for hiding this comment

watilde Feb 16, 2017

Choose a reason for hiding this comment

watilde Feb 16, 2017

Choose a reason for hiding this comment

TimothyGu commented Feb 16, 2017

TimothyGu commented Feb 16, 2017

jasnell commented Feb 16, 2017

jasnell commented Feb 19, 2017

joyeecheung Feb 15, 2017 •

edited

Loading