Faster ascii string case conversion.

Faster ascii string case conversion.

Committed: http://code.google.com/p/v8/source/detail?r=5713

[antonm, 2010-10-26 17:17:33]

LGTM.

Pretty neat.

http://codereview.chromium.org/4189001/diff/3001/4002
File src/runtime.cc (right):

http://codereview.chromium.org/4189001/diff/3001/4002#newcode4644
src/runtime.cc:4644: // the range (m, n). All the other bits in the result are
cleared.
maybe follow more standard [;) intervals

http://codereview.chromium.org/4189001/diff/3001/4002#newcode4647
src/runtime.cc:4647: // Requires: the input word and the boundaries must be
ascii.
I think ascii requires additional clarification.  And maybe it is better to
formulate it as a restriction on the range of values

http://codereview.chromium.org/4189001/diff/3001/4002#newcode4684
src/runtime.cc:4684: while (src <= limit - sizeof(uintptr_t)) {
Maybe convert src to uintptr_t*?  That would allow to get rid of the casts and
sizeof's.

http://codereview.chromium.org/4189001/diff/3001/4003
File test/mjsunit/string-case.js (right):

http://codereview.chromium.org/4189001/diff/3001/4003#newcode55
test/mjsunit/string-case.js:55: function randomGenerator(i, j) {
I don't think you have to declare unused arguments here (at least JS allows
that).  I find them somewhat misleading, but up to you.

http://codereview.chromium.org/4189001/diff/3001/4003#newcode77
test/mjsunit/string-case.js:77: test(generator, i + j);
I am slightly concerned that string pattern for this case is too regular, for
example, you only cases when string starts with lower case chars (0 or more)
than has a region of upper case chars and again lower case.  Maybe I'd only
leave pseudo-random generator, but up to you.

[Vitaly Repeshko, 2010-10-26 18:14:47]

Thanks for speedy review! Comments addressed. Patch submitted.


-- Vitaly

http://codereview.chromium.org/4189001/diff/3001/4002
File src/runtime.cc (right):

http://codereview.chromium.org/4189001/diff/3001/4002#newcode4644
src/runtime.cc:4644: // the range (m, n). All the other bits in the result are
cleared.
On 2010/10/26 17:17:33, antonm wrote:
> maybe follow more standard [;) intervals

I want to make usages of m and n similar, so either (;) or [;].

http://codereview.chromium.org/4189001/diff/3001/4002#newcode4647
src/runtime.cc:4647: // Requires: the input word and the boundaries must be
ascii.
On 2010/10/26 17:17:33, antonm wrote:
> I think ascii requires additional clarification.  And maybe it is better to
> formulate it as a restriction on the range of values

Agreed. Extended the comment.

http://codereview.chromium.org/4189001/diff/3001/4002#newcode4684
src/runtime.cc:4684: while (src <= limit - sizeof(uintptr_t)) {
On 2010/10/26 17:17:33, antonm wrote:
> Maybe convert src to uintptr_t*?  That would allow to get rid of the casts and
> sizeof's.

Ahh, I'm really afraid of the compiler emitting different code.

http://codereview.chromium.org/4189001/diff/3001/4003
File test/mjsunit/string-case.js (right):

http://codereview.chromium.org/4189001/diff/3001/4003#newcode55
test/mjsunit/string-case.js:55: function randomGenerator(i, j) {
On 2010/10/26 17:17:33, antonm wrote:
> I don't think you have to declare unused arguments here (at least JS allows
> that).  I find them somewhat misleading, but up to you.

Obsolete.

http://codereview.chromium.org/4189001/diff/3001/4003#newcode77
test/mjsunit/string-case.js:77: test(generator, i + j);
On 2010/10/26 17:17:33, antonm wrote:
> I am slightly concerned that string pattern for this case is too regular, for
> example, you only cases when string starts with lower case chars (0 or more)
> than has a region of upper case chars and again lower case.  Maybe I'd only
> leave pseudo-random generator, but up to you.

You're right. I dropped the half-baked deterministic part of the test and
increased the number of passes to exercise more inputs using the random number
generator. A nice side effect of this is that the test no longer timeouts on
ARM.