Bug 118415 – A lot of code duplication within StringImpl 'equal' functions

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Mikhail Pozdnyakov]

SSIA.

[Mikhail Pozdnyakov]

Created attachment 206153 [details]
patch

[Mikhail Pozdnyakov]

Comment on attachment 206153 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206153&action=review

> Source/WTF/wtf/text/StringImpl.h:869
> +ALWAYS_INLINE bool arraysEqual(const U* a, const V* b, unsigned length)

maybe there is a better place for this function..

[Anders Carlsson]

Comment on attachment 206153 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206153&action=review

>> Source/WTF/wtf/text/StringImpl.h:869
>> +ALWAYS_INLINE bool arraysEqual(const U* a, const V* b, unsigned length)
> 
> maybe there is a better place for this function..

I think you can just use std::equal here instead.

[Mikhail Pozdnyakov]

(In reply to comment #3)
> (From update of attachment 206153 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206153&action=review
> 
> >> Source/WTF/wtf/text/StringImpl.h:869
> >> +ALWAYS_INLINE bool arraysEqual(const U* a, const V* b, unsigned length)
> > 
> > maybe there is a better place for this function..
> 
> I think you can just use std::equal here instead.
Indeed! Thanks for proposal!

[Mikhail Pozdnyakov]

Created attachment 206156 [details]
patch v2

Using std::equal.

[WebKit Commit Bot]

Comment on attachment 206156 [details]
patch v2

Clearing flags on attachment: 206156

Committed r152418: <http://trac.webkit.org/changeset/152418>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

The second half of the patch is a bad change IMHO.

Those two:
   bool equal(const LChar* a, const LChar* b, unsigned length)
   bool equal(const UChar* a, const UChar* b, unsigned length)
are the slow path not used by anyone. They were kept like that for clarity.

I you want to replace them, it should be with memcmp, not std::equal.


For
   bool equal(const LChar* a, const UChar* b, unsigned length)
   bool equal(const UChar* a, const LChar* b, unsigned length)
chances is are you will cause worst code to be generated.

[Anders Carlsson]

(In reply to comment #8)
> The second half of the patch is a bad change IMHO.
> 
> Those two:
>    bool equal(const LChar* a, const LChar* b, unsigned length)
>    bool equal(const UChar* a, const UChar* b, unsigned length)
> are the slow path not used by anyone. They were kept like that for clarity.
> 
> I you want to replace them, it should be with memcmp, not std::equal.

The compiler will optimize std::equal to memcmp.

[Benjamin Poulain]

(In reply to comment #9)
> The compiler will optimize std::equal to memcmp.

No it won't.

[Mikhail Pozdnyakov]

(In reply to comment #8)
> The second half of the patch is a bad change IMHO.
> 
I wouldn't say it's a bad change, yes performance is not improved and it is the same as before, but the code became more compact and clear (and not duplicated) which was the goal of the patch.

[Benjamin Poulain]

> I wouldn't say it's a bad change, yes performance is not improved and it is the same as before, but the code became more compact and clear (and not duplicated) which was the goal of the patch.

I don't know why you would say that.

Obviously the performance is gonna be worse than before.

[Anders Carlsson]

(In reply to comment #12)
> > I wouldn't say it's a bad change, yes performance is not improved and it is the same as before, but the code became more compact and clear (and not duplicated) which was the goal of the patch.
> 
> I don't know why you would say that.
> 
> Obviously the performance is gonna be worse than before.

I don’t think the std::equal loop is slower than the for loop - in a test program I see identical code being generated with clang.

[Benjamin Poulain]

(In reply to comment #13)
> I don’t think the std::equal loop is slower than the for loop - in a test program I see identical code being generated with clang.

Did you use a compile time constant as your length parameter?
If not, I don't see how the compiler will optimize that code.

Okay, I gave that a shot.
No matter which optimization level I chose, Clang always gives me this for std::equal():
0000bfc0            2301        movs    r3, #0x1
0000bfc2            b172        cbz     r2, 0xbfe2
0000bfc4        f8919000        ldrb.w  r9, [r1]
0000bfc8            2300        movs    r3, #0x0
0000bfca        f890c000        ldrb.w  r12, [r0]
0000bfce            45cc        cmp     r12, r9
0000bfd0            d107        bne     0xbfe2
0000bfd2            3a01        subs    r2, #0x1
0000bfd4        f1010101        add.w   r1, r1, #0x1
0000bfd8        f1000001        add.w   r0, r0, #0x1
0000bfdc        f04f0301        mov.w   r3, #0x1
0000bfe0            d1f0        bne     0xbfc4
0000bfe2            4618        mov     r0, r3
0000bfe4            4770        bx      lr
0000bfe6            bf00        nop