Bug 74063 – Improve charactersAreAllASCII() to compare multiple characters at a time

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Benjamin Poulain]

We could load a word size of character at every iteration...
Memory access would be more effective, and we would need less iterations in the loop to go over the string.

[Benjamin Poulain]

Created attachment 118344 [details]
Patch

[Benjamin Poulain]

This is more for fun than anything.

I measured the gain of the function itself, it is a bit more than the expected 4x, 8x, 16x. I did not really look into it but I guess that is because of the reduced number of memory access.

The function get a lot more complicated than the original, so I am not sure that is worth it for the project.

[Early Warning System Bot]

Comment on attachment 118344 [details]
Patch

Attachment 118344 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10797174

[Darin Adler]

Comment on attachment 118344 [details]
Patch

The functions in TextCodecASCIIFastPath.h provide a more readable way to do this type of optimization for single-byte characters, and ideally I’d like to see this share code with that. Maybe we can move those functions into WTF?

[Benjamin Poulain]

> The functions in TextCodecASCIIFastPath.h provide a more readable way to do this type of optimization for single-byte characters, and ideally I’d like to see this share code with that. Maybe we can move those functions into WTF?

I was unaware the mask were available in TextCodecASCIIFastPath.h. I can have a look at moving the file to WTF.

[Benjamin Poulain]

Created attachment 118751 [details]
Patch

[Early Warning System Bot]

Comment on attachment 118751 [details]
Patch

Attachment 118751 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10836058

[Benjamin Poulain]

Created attachment 118838 [details]
Patch

[Benjamin Poulain]

Now that ASCIIType.h is no longer bloated, what do you think about merging the ASCIIFastPathHelper.h in this patch in ASCIIType?

[Darin Adler]

(In reply to comment #9)
> Now that ASCIIType.h is no longer bloated, what do you think about merging the ASCIIFastPathHelper.h in this patch in ASCIIType?

I don’t think this belongs in <wtf/ASCIICType.h>. The point of <ASCIICType.h> is to offer functions like the ones in <ctype.h>, but ones that work with ASCII. It should not have other functions in it. In fact, I believe some of the functions creeping in there do not belong there.

I do think its’s good to have some functions for working with ASCII all in one header. It could even be named <wtf/ASCII.h>. I don’t like the word “helper” in a header name. Headers all help you!

[Darin Adler]

Comment on attachment 118838 [details]
Patch

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 <[e-mail]>

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.

[Benjamin Poulain]

ASCIIFastPath.h it is then :)

> > Source/JavaScriptCore/wtf/text/ASCIIFastPathHelper.h:4
> > + * Copyright (C) 2011 Benjamin Poulain <[e-mail]>
> 
> 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.

I use to have the distinction between work done...at work, and what I do on my weekends at home. I'll remove this.

[Benjamin Poulain]

Created attachment 121030 [details]
Patch

[Andreas Kling]

Comment on attachment 121030 [details]
Patch

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

> Source/JavaScriptCore/wtf/text/ASCIIFastPath.h:2
> + * Copyright (C) 2011 Apple Inc. All rights reserved.

Needs more 2012!

[Benjamin Poulain]

All the bots are red, I'll wait a bit before landing this.

[WebKit Review Bot]

Comment on attachment 121030 [details]
Patch

Clearing flags on attachment: 121030

Committed r104127: <http://trac.webkit.org/changeset/104127>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.