code review 2317044: Optimization of the frequently used strequal. Checks th...

Optimization of the frequently used strequal. Checks the relative length of the two strings, then delegates to memequal.  memequal has been changed to use pointer math.

[bsiegert, 2010-10-06 09:27:57]

http://codereview.appspot.com/2317044/diff/1/src/pkg/runtime/runtime.c
File src/pkg/runtime/runtime.c (right):

http://codereview.appspot.com/2317044/diff/1/src/pkg/runtime/runtime.c#newcod...
src/pkg/runtime/runtime.c:319: while (ba != aend) {
Just a nitpick: no space before the opening parenthesis.

http://codereview.appspot.com/2317044/diff/1/src/pkg/runtime/runtime.c#newcod...
src/pkg/runtime/runtime.c:398: if (aLen != b->len) {
No space here too, plus you can omit the curly braces.

[gmiller, 2010-10-06 13:03:28]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[gmiller, 2010-10-06 13:25:24]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc1, 2010-10-06 14:05:39]

few more nits

http://codereview.appspot.com/2317044/diff/8001/src/pkg/runtime/runtime.c
File src/pkg/runtime/runtime.c (right):

http://codereview.appspot.com/2317044/diff/8001/src/pkg/runtime/runtime.c#new...
src/pkg/runtime/runtime.c:397: int32 aLen = a->len;
style is declarations at top of function, no init, then blank line.  also mixed
case not so much here (unlike Go).

int32 alen;

USED(s);
alen = a->len;

the casts to (void*) are unnecessary,
as are the parens around (a->str) and (b->str)
(even with the casts the parens would be
unnecessary).

[gmiller, 2010-10-06 14:18:16]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2010-10-06 14:36:29]

looking better, but you missed two of my comments:

blank line before USED(s).
s/aLen/alen/

russ

[gmiller, 2010-10-06 14:47:00]

Sorry, not enough coffee this morning.  Will resubmit.  (fingers crossed)

On 2010/10/06 14:36:29, rsc wrote:
> looking better, but you missed two of my comments:
> 
> blank line before USED(s).
> s/aLen/alen/
> 
> russ

[gmiller, 2010-10-06 14:52:28]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2010-10-06 15:13:44]

LGTM

Will submit once I've added you to the contributors list.
Thanks.

[cwvh, 2010-10-06 17:29:39]

http://codereview.appspot.com/2317044/diff/3/src/pkg/runtime/runtime.c
File src/pkg/runtime/runtime.c (right):

http://codereview.appspot.com/2317044/diff/3/src/pkg/runtime/runtime.c#newcod...
src/pkg/runtime/runtime.c:402: return memequal(alen, a->str, b->str);
Why introduce the redundant variable to save two keys? The "return false" seems
bizarre since the return type is uint32 and the original code returned explicit
integers. I think you should use Russ' example from the go-nuts message:

return a->len == b->len && memequal(a->len, a->str, b->str);

The above seems more clear, concise, and similar to the style of C used in the
project.

[gmiller, 2010-10-06 17:46:34]

Hello rsc, cwvh (cc: golang-dev@googlegroups.com),

Please take another look.

[gmiller, 2010-10-06 17:53:43]

http://codereview.appspot.com/2317044/diff/3/src/pkg/runtime/runtime.c
File src/pkg/runtime/runtime.c (right):

http://codereview.appspot.com/2317044/diff/3/src/pkg/runtime/runtime.c#newcod...
src/pkg/runtime/runtime.c:402: return memequal(alen, a->str, b->str);
On 2010/10/06 17:29:39, cwvh wrote:
> Why introduce the redundant variable to save two keys? 
Actually it's to save the expense of the lookup, not the keystrokes. 

> The "return false" seems
> bizarre since the return type is uint32 and the original code returned
explicit
> integers. 
Agreed.

[rsc, 2010-10-07 07:13:27]

*** Submitted as http://code.google.com/p/go/source/detail?r=3be74abce2a4 ***

runtime: faster strequal, memequal

Fixes issue 1161.

R=rsc, cwvh
CC=golang-dev
http://codereview.appspot.com/2317044

Committer: Russ Cox <rsc@golang.org>