Bug 99363 – StringImpl::reverseFind() with a single match character isn't optimal for mixed 8/16 bit cases

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

StringImpl::reverseFind() has code to handle single character matches optimally if both the subject and match strings are the same bit-ness.  The code should be optimized for all four bit ness combinations.

[Michael Saboff]

Created attachment 168778 [details]
Patch

[Benjamin Poulain]

Comment on attachment 168778 [details]
Patch

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

> Source/WTF/wtf/text/StringImpl.cpp:1145
> +        return WTF::reverseFind(characters(), ourLength, matchCharacter, index);

characters() -> characters16()

[Michael Saboff]

(In reply to comment #2)
> (From update of attachment 168778 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168778&action=review
> 
> > Source/WTF/wtf/text/StringImpl.cpp:1145
> > +        return WTF::reverseFind(characters(), ourLength, matchCharacter, index);
> 
> characters() -> characters16()

Done in my sandbox, want another patch?

[Benjamin Poulain]

> > characters() -> characters16()
> 
> Done in my sandbox, want another patch?

That's ok, that was just a quick comment before going out. I'll have a look at your patch.

[Benjamin Poulain]

Comment on attachment 168778 [details]
Patch

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

> Source/WTF/wtf/text/StringImpl.cpp:1035
> +        UChar matchCharacter = matchString->is8Bit() ? matchString->characters8()[0] : matchString->characters16()[0];

StringImpl's operator[] does that for you.

> Source/WTF/wtf/text/StringImpl.cpp:-1135
> -        if (is8Bit() && matchString->is8Bit())
> -            return WTF::reverseFind(characters8(), ourLength, matchString->characters8()[0], index);
> -        return WTF::reverseFind(characters(), ourLength, matchString->characters()[0], index);

This was intentional as the reverseFind(LChar*, UChar) was not implemented. Then I forgot about it.

Does WTF::reverseFind() has this combination now? I don't see it in StringImpl.h.

> Source/WTF/wtf/text/StringImpl.cpp:1141
> +        UChar matchCharacter = matchString->is8Bit() ? matchString->characters8()[0] : matchString->characters16()[0];

see StringImpl's operator[].

[Michael Saboff]

Created attachment 168832 [details]
Patch with review updates

(In reply to comment #5)
> (From update of attachment 168778 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168778&action=review
> 
> > Source/WTF/wtf/text/StringImpl.cpp:1035
> > +        UChar matchCharacter = matchString->is8Bit() ? matchString->characters8()[0] : matchString->characters16()[0];
> 
> StringImpl's operator[] does that for you.

Fixed.  I think I added operator[].

> > Source/WTF/wtf/text/StringImpl.cpp:-1135
> > -        if (is8Bit() && matchString->is8Bit())
> > -            return WTF::reverseFind(characters8(), ourLength, matchString->characters8()[0], index);
> > -        return WTF::reverseFind(characters(), ourLength, matchString->characters()[0], index);
> 
> This was intentional as the reverseFind(LChar*, UChar) was not implemented. Then I forgot about it.
> 
> Does WTF::reverseFind() has this combination now? I don't see it in StringImpl.h.

I updated reverseFind() to be like find().

> > Source/WTF/wtf/text/StringImpl.cpp:1141
> > +        UChar matchCharacter = matchString->is8Bit() ? matchString->characters8()[0] : matchString->characters16()[0];
> 
> see StringImpl's operator[].

[Benjamin Poulain]

Comment on attachment 168832 [details]
Patch with review updates

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

> Source/WTF/wtf/text/StringImpl.h:1015
> +ALWAYS_INLINE size_t reverseFind(const UChar* characters, unsigned length, LChar matchCharacter, unsigned index = 0)

default value of index looks wrong.

> Source/WTF/wtf/text/StringImpl.h:1020
> +inline size_t reverseFind(const LChar* characters, unsigned length, UChar matchCharacter, unsigned index = 0)

Ditto.

[Darin Adler]

Comment on attachment 168832 [details]
Patch with review updates

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

> Source/WTF/wtf/text/StringImpl.h:1022
> +    if (matchCharacter & ~0xFF)

Not happy with the magic math here. There’s a reason we have a named function isASCII for the case like this with 0x7F, and I’d prefer a named functin for this kind of use too.

[Michael Saboff]

Committed r131524: <http://trac.webkit.org/changeset/131524>