SSIA.
Created attachment 206153 [details] patch
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..
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.
(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!
Created attachment 206156 [details] patch v2 Using std::equal.
Comment on attachment 206156 [details] patch v2 Clearing flags on attachment: 206156 Committed r152418: <http://trac.webkit.org/changeset/152418>
All reviewed patches have been landed. Closing bug.
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.
(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.
(In reply to comment #9) > The compiler will optimize std::equal to memcmp. No it won't.
(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.
> 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.
(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.
(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