[webkit-reviews] review granted: [Bug 74063] Improve charactersAreAllASCII() to compare multiple characters at a time : [Attachment 118838] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 74063: Improve charactersAreAllASCII() to compare multiple characters at a
time
https://bugs.webkit.org/show_bug.cgi?id=74063

Attachment 118838: Patch
https://bugs.webkit.org/attachment.cgi?id=118838&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=118838&action=review


Did you measure the performance impact of this? The code is faster for long
strings but I suspect it’s slower for short ones.

> Source/JavaScriptCore/wtf/text/ASCIIFastPathHelper.h:4
> + * Copyright (C) 2011 Benjamin Poulain <benjamin at webkit.org>

If you are doing this work as an Apple employee you should not add your own
copyright to the file. If you did the work when not an Apple employee that
might be OK.

> Source/JavaScriptCore/wtf/text/ASCIIFastPathHelper.h:46
> +
> +

We normally don’t use multiple blank lines to paragraph. We use just one.

If you want a larger separator you’d normally use a comment.

> Source/JavaScriptCore/wtf/text/ASCIIFastPathHelper.h:47
> +template<size_t size, typename CharType> struct NonASCIIMask;

I’d prefer that “character type” not be abbreviated to CharType. How about
CharacterType? I would like that in ASCIICType.h too.

> Source/JavaScriptCore/wtf/text/ASCIIFastPathHelper.h:69
> +// Note: this function assume the input is likely all ASCII, and
> +// do not leave early if it is not the case.

The word “this” should be capitalized.

The word “do” should be “does”.

> Source/JavaScriptCore/wtf/text/ASCIIFastPathHelper.h:80
> +    // Prologue: align the input.
> +    while (!isAlignedToMachineWord(characters) && (characters != end)) {
> +	   allCharBits |= *characters;
> +	   ++characters;
> +    }

We may need to find a way to incorporate a fast path for buffers that are known
to be aligned to a machine word that omits this first loop. For example, I
suspect all string buffers for StringImpl have such a guarantee.

I don’t think the parentheses are characters != end are needed.

> Source/JavaScriptCore/wtf/text/ASCIIFastPathHelper.h:83
> +    const CharType *wordEnd = alignToMachineWord(end);

Misplaced * here.