Skip to content

Commit

Permalink
test: improve performance of stringbytes test
Browse files Browse the repository at this point in the history
String concatenation in the assert messages has drastic impact on test
runtime. Removal of these messages is unlikely to affect debugging if
any breaking changes are made.

Previous time to run:

    $ time ./iojs test/parallel/test-stringbytes-external.js

    real    0m2.321s
    user    0m2.256s
    sys     0m0.092s

With fix:

    $ time ./iojs test/parallel/test-stringbytes-external.js

    real    0m0.518s
    user    0m0.508s
    sys     0m0.008s

PR-URL: #2544
Reviewed-By: trevnorris - Trevor Norris <trev.norris@gmail.com>
Reviewed-By: thefourtheye - Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
  • Loading branch information
trevnorris authored and orangemocha committed Sep 1, 2015
1 parent 5169b51 commit 37ee43e
Showing 1 changed file with 5 additions and 17 deletions.
22 changes: 5 additions & 17 deletions test/parallel/test-stringbytes-external.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,7 @@ var c_ucs = new Buffer(b_ucs, 'ucs2');
assert.equal(c_bin.length, c_ucs.length);
// make sure Buffers from externals are the same
for (var i = 0; i < c_bin.length; i++) {
assert.equal(c_bin[i], c_ucs[i], c_bin[i] + ' == ' + c_ucs[i] +
' : index ' + i);
assert.equal(c_bin[i], c_ucs[i]);
}
// check resultant strings
assert.equal(c_bin.toString('ucs2'), c_ucs.toString('ucs2'));
Expand All @@ -63,19 +62,14 @@ var PRE_3OF4_APEX = Math.ceil((EXTERN_APEX / 4) * 3) - RADIOS;
var pumped_string2 = slice2.toString('hex');
var decoded = new Buffer(pumped_string, 'hex');

var metadata = '\nEXTERN_APEX=1031913 - pumped_string.length=';
metadata += pumped_string.length + '\n';

// the string are the same?
for (var k = 0; k < pumped_string.length; ++k) {
assert.equal(pumped_string[k], pumped_string2[k],
metadata + 'chars should be the same at ' + k);
assert.equal(pumped_string[k], pumped_string2[k]);
}

// the recoded buffer is the same?
for (var i = 0; i < decoded.length; ++i) {
assert.equal(datum[i], decoded[i],
metadata + 'bytes should be the same at ' + i);
assert.equal(datum[i], decoded[i]);
}
}
})();
Expand All @@ -89,20 +83,14 @@ var PRE_3OF4_APEX = Math.ceil((EXTERN_APEX / 4) * 3) - RADIOS;
var pumped_string2 = slice2.toString('base64');
var decoded = new Buffer(pumped_string, 'base64');

var metadata = '\nEXTERN_APEX=1031913 - data=" + slice.length';
metadata += ' pumped_string.length=' + pumped_string.length + '\n';

// the string are the same?
for (var k = 0; k < pumped_string.length - 3; ++k) {
assert.equal(pumped_string[k], pumped_string2[k],
metadata + 'chars should be the same for two slices at '
+ k + ' ' + pumped_string[k] + ' ' + pumped_string2[k]);
assert.equal(pumped_string[k], pumped_string2[k]);
}

// the recoded buffer is the same?
for (var i = 0; i < decoded.length; ++i) {
assert.equal(datum[i], decoded[i],
metadata + 'bytes should be the same at ' + i);
assert.equal(datum[i], decoded[i]);
}
}
})();
Expand Down

5 comments on commit 37ee43e

@trevnorris
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@orangemocha if you're going to totally screw with someone's commit make sure you fill in the fields correctly. This commit is for PR #2410.

@orangemocha
Copy link
Contributor

Choose a reason for hiding this comment

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

@trevnorris sorry, I was trying to help, not sure what happened. Did you get my emails?
It looks like both commits were on that branch: https://github.com/nodejs/node/compare/reland-landed-commitss

@orangemocha
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm ok, I see, it looks like you meant to land both commits with one job, but they had different PR-URL. That's currently not possible, because the merge job always strips any existing PR-URL and Reviewed-By, then tries to stick the one in the form. I'll make a change to support this scenario. Sorry about the confusion.

@trevnorris
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@orangemocha My comment was overly reactive. My apologies. After getting these two commits re-landed was surprised/frustrated to see that the PR-URL field had been altered. Though now I understand much better how the CI works.

@orangemocha
Copy link
Contributor

Choose a reason for hiding this comment

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

@trevnorris no worries. It's good feedback: nodejs/build#179. Please chip in with any suggestions in there. Until it's fixed, please let me know if you need any help working around this limitation.

Please sign in to comment.