Bug 222396 – [WTF] String length check in equal should be inlined

• Component: New Bugs
• Status: NEW

[Yusuke Suzuki]

[WTF] String length check in equal should be inlined

[Yusuke Suzuki]

Created attachment 421491 [details]
Patch

[Darin Adler]

Comment on attachment 421491 [details]
Patch

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

I have a few comments, but no idea why all the tests are failing. Wasn’t able to spot the mistake.

> Source/WTF/wtf/text/StringImpl.cpp:1582
> +bool equalInternal(const StringImpl& a, const StringImpl& b, unsigned length)

Not sure it’s relevant but the code below would work on StringView too, not just const StringImpl&.

> Source/WTF/wtf/text/StringImpl.h:580
> +inline bool equal(const StringImpl& a, const StringImpl& b)

Can we separate the declarations from the definitions? Lets put all the function bodies after we declare all the functions. It would be nice to be able to see what’s available without it being interspersed with all the implementation details.

> Source/WTF/wtf/text/StringImpl.h:585
> +    if (length != b.length())
> +        return false;
> +    return equalInternal(a, b, length);

I like the idea of using && rather than early return:

    return length == b.length() && equalInternal(a, b, length);

You might prefer it as-is, though.

> Source/WTF/wtf/text/StringImpl.h:594
> +    if (a == b)
> +        return true;
> +    if (!a || !b)
> +        return false;
> +    return equal(*a, *b);

Even here I am a big fan of boolean expressions.

    return a == b || (a && b && equal(*a, *b));

Just seems clearer to me.

> Source/WTF/wtf/text/StringImpl.h:603
> +template<typename CharacterType>
> +inline bool equalInternal(const StringImpl& a, const CharacterType& b, unsigned length)
> +{
> +    if (a.length() != length)
> +        return false;
> +    return equalInternal(a, b, length);
> +}

This is really confusing to me. In other equalInternal functions the caller supplies a length that has to be correct for both. But in this one, it seems the length is only the length of b. At the very least I would suggest naming this "bLength". I think we need to distinguish these subtleties with names as well as we can.

Maybe some of the equalInternal should be named equalWithoutLengthCheck.

And I don’t see why this one needs to be "internal". Maybe what makes this internal is that we don’t want templates to interfere with overload resolution?

> Source/WTF/wtf/text/StringImpl.h:606
> +inline bool equalInternal(const StringImpl* a, const CharacterType* b, unsigned length)

Like above, I think the length argument needs to be named "bLength".

And I don’t see why this one needs to be "internal". Maybe what makes this internal is that we don’t want templates to interfere with overload resolution?

> Source/WTF/wtf/text/StringImpl.h:612
> +    if (!a)
> +        return !b;
> +    if (!b)
> +        return false;
> +    return equalInternal(*a, *b, length);

For clarity, I would write:

    return (!a && !b) || (a && b && equalInternal(*a, *b, length));

Unless the compiler can’t generate code that’s equally efficient for this, in which case it’s fine to do the other.

[Aakash Jain]

This resulted in over 175 Million log lines in https://ews-build.webkit.org/#/builders/1/builds/33358 while running jsc tests.

[Aakash Jain]

Similarly over 20 million lines in https://ews-build.webkit.org/#/builders/46/builds/3603

[Radar WebKit Bug Importer]

<rdar://problem/75014237>