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.
Created attachment 118344 [details] Patch
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.
Comment on attachment 118344 [details] Patch Attachment 118344 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10797174
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?
> 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.
Created attachment 118751 [details] Patch
Comment on attachment 118751 [details] Patch Attachment 118751 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10836058
Created attachment 118838 [details] Patch
Now that ASCIIType.h is no longer bloated, what do you think about merging the ASCIIFastPathHelper.h in this patch in ASCIIType?
(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!
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 <benjamin@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.
ASCIIFastPath.h it is then :) > > Source/JavaScriptCore/wtf/text/ASCIIFastPathHelper.h:4 > > + * Copyright (C) 2011 Benjamin Poulain <benjamin@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. I use to have the distinction between work done...at work, and what I do on my weekends at home. I'll remove this.
Created attachment 121030 [details] Patch
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!
All the bots are red, I'll wait a bit before landing this.
Comment on attachment 121030 [details] Patch Clearing flags on attachment: 121030 Committed r104127: <http://trac.webkit.org/changeset/104127>
All reviewed patches have been landed. Closing bug.