Bug 75821 - Using strncmp() for comparing scheme and port numbers is inefficient
Summary: Using strncmp() for comparing scheme and port numbers is inefficient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-08 20:01 PST by Benjamin Poulain
Modified: 2012-02-18 14:51 PST (History)
4 users (show)

See Also:


Attachments
Patch (6.17 KB, patch)
2012-01-08 20:09 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (6.23 KB, patch)
2012-01-14 20:40 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (6.05 KB, patch)
2012-01-25 17:37 PST, Benjamin Poulain
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2012-01-08 20:01:02 PST
In KURL, we frequently compare very short strings, like the scheme and port.
Currently, we use strncmp(), we could just compare char by char and let the compiler inline everything.
Comment 1 Benjamin Poulain 2012-01-08 20:09:18 PST
Created attachment 121613 [details]
Patch
Comment 2 Darin Adler 2012-01-08 20:44:17 PST
Comment on attachment 121613 [details]
Patch

You should be able to do all of this without hard-coding the lengths of the arrays.

template<size_t bLength> static inline bool equal(const char* a, const char b[bLength])

Try it.
Comment 3 Benjamin Poulain 2012-01-09 22:27:22 PST
Comment on attachment 121613 [details]
Patch

I'll update later this week, I am busy with another bug atm :(
Comment 4 Benjamin Poulain 2012-01-14 20:40:50 PST
Created attachment 122562 [details]
Patch
Comment 5 Benjamin Poulain 2012-01-14 20:42:00 PST
The template "template<size_t bLength> static inline bool equal(const char* a, const char b[bLength])" never matched.
I read online to use a reference to the array, and this time the compiler match the template.
Comment 6 Benjamin Poulain 2012-01-25 17:37:03 PST
Created attachment 124046 [details]
Patch
Comment 7 Darin Adler 2012-01-26 11:00:36 PST
Comment on attachment 124046 [details]
Patch

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

> Source/WebCore/platform/KURL.cpp:1079
> +template<size_t referenceLength>
> +static inline bool equal(const char* str, size_t length, const char (&reference)[referenceLength])
> +{
> +    return length == referenceLength && equal(str, reference);
>  }

This compares two strings. It seems a little strange that the first string is named “str” with length “length” and the second is named “reference” with name “referenceLength”. I would just name them “a” and “b”, or “stringA” and “stringB”, and “lengthA” and “lengthB”, or something like that.
Comment 8 Benjamin Poulain 2012-01-26 11:03:38 PST
> This compares two strings. It seems a little strange that the first string is named “str” with length “length” and the second is named “reference” with name “referenceLength”. I would just name them “a” and “b”, or “stringA” and “stringB”, and “lengthA” and “lengthB”, or something like that.

Stricto sensu, it is comparing the string to an (reference) const array, that is why I had chosen the name "reference". I do not mind updating that :)

Thanks for the review!
Comment 9 Anders Carlsson 2012-01-26 11:15:44 PST
For reference, I still don't think it's necessary to declare the strings as character arrays, just make equals handle the trailing null character.
Comment 10 Benjamin Poulain 2012-01-26 12:10:09 PST
Committed r106025: <http://trac.webkit.org/changeset/106025>
Comment 11 Darin Adler 2012-01-31 10:31:37 PST
(In reply to comment #9)
> For reference, I still don't think it's necessary to declare the strings as character arrays, just make equals handle the trailing null character.

Right, we could easily do it that way which would look a little nicer.
Comment 12 Benjamin Poulain 2012-02-18 14:51:47 PST
Comment on attachment 124046 [details]
Patch

Clearing useless flag.