Implement fast path in UTF8ToUTF16 for pure ASCII strings

Implement fast path in UTF8ToUTF16 for pure ASCII strings

Blink WebString::fromUTF8 are marginally faster than UTF8ToUTF16 on
pure ASCII input because they have a fast path for this case. Implement
the same thing for base::UTF8ToUTF16

DoIsStringASCII is now a port of charactersAreAllASCII from Blink.

BUG=391492
R=brettw@chromium.org, torne@chromium.org

Committed: https://crrev.com/6f500092b9396517a40fdf1ca6a5199738e673e9
Cr-Commit-Position: refs/heads/master@{#297158}

[mnaganov (inactive), 2014-09-05 12:22:35]

mnaganov@chromium.org changed reviewers:
+ torne@chromium.org

[mnaganov (inactive), 2014-09-05 12:22:35]

Torne, can you please take a look?

[Torne, 2014-09-05 13:40:51]

Code LGTM, but are we sure that the other callers of IsStringASCII expect it to
be true most of the time?

[mnaganov (inactive), 2014-09-05 14:29:38]

On 2014/09/05 13:40:51, Torne wrote:
> Code LGTM, but are we sure that the other callers of IsStringASCII expect it
to
> be true most of the time?

As I have noted in the bug, in reality about 75% of strings fed to UTF8ToUTF16
are in fact pure ASCII. Although, this figure is based on my run of an
instrumented version of Chrome where I was doing my usual stuff.

[mnaganov (inactive), 2014-09-05 14:32:09]

mnaganov@chromium.org changed reviewers:
+ brettw@chromium.org

[mnaganov (inactive), 2014-09-05 14:32:10]

Hi Brett,

Can you please take a look at this change? Please see the related bug for more
context.

[mnaganov (inactive), 2014-09-05 14:35:08]

On 2014/09/05 14:29:38, mnaganov (cr) wrote:
> On 2014/09/05 13:40:51, Torne wrote:
> > Code LGTM, but are we sure that the other callers of IsStringASCII expect it
> to
> > be true most of the time?
> 
> As I have noted in the bug, in reality about 75% of strings fed to UTF8ToUTF16
> are in fact pure ASCII. Although, this figure is based on my run of an
> instrumented version of Chrome where I was doing my usual stuff.

Oh, sorry, I didn't answer your question in full. As far as I can see from the
code,
most of the calls to IsStringASCII are made with the assumption that the string
is
ASCII.

[Torne, 2014-09-05 14:42:55]

OK, seems reasonable to me too from a quick look through codesearch; most uses
seem to be "assert that this is ascii and fail if not", so optimising for the
ascii case looks ideal.

[mnaganov (inactive), 2014-09-10 07:56:17]

brettw@: Brett, can you please take a look? git blame shows that you authored
utf_string_conversions.cc (although long time ago :)

[mnaganov (inactive), 2014-09-15 10:38:25]

On 2014/09/10 07:56:17, mnaganov (cr) wrote:
> brettw@: Brett, can you please take a look? git blame shows that you authored
> utf_string_conversions.cc (although long time ago :)

brettw@: Friendly ping!

[brettw, 2014-09-16 17:14:28]

I want to be sure to get unit tests for this showing it working for various
alignments.

https://codereview.chromium.org/543043002/diff/1/base/strings/string_util.cc
File base/strings/string_util.cc (right):

https://codereview.chromium.org/543043002/diff/1/base/strings/string_util.cc#...
base/strings/string_util.cc:70: const uintptr_t machineWordAlignmentMask =
sizeof(MachineWord) - 1;
Constant naming: kMachineWord....

https://codereview.chromium.org/543043002/diff/1/base/strings/string_util.h
File base/strings/string_util.h (right):

https://codereview.chromium.org/543043002/diff/1/base/strings/string_util.h#n...
base/strings/string_util.h:254: BASE_EXPORT bool IsStringASCII(const char* src,
size_t src_len);
The string piece version should cover this case. Actually, I think we should
have a a StringPiece and a StringPiece16 version of this function and nothing
else. StringPiece is supposed to handle exactly what you're doing.

[mnaganov (inactive), 2014-09-17 15:15:02]

Brett, thanks for your suggestions! I added a test covering various alignment
cases.

https://codereview.chromium.org/543043002/diff/1/base/strings/string_util.cc
File base/strings/string_util.cc (right):

https://codereview.chromium.org/543043002/diff/1/base/strings/string_util.cc#...
base/strings/string_util.cc:70: const uintptr_t machineWordAlignmentMask =
sizeof(MachineWord) - 1;
On 2014/09/16 17:14:28, brettw wrote:
> Constant naming: kMachineWord....

Done.

https://codereview.chromium.org/543043002/diff/1/base/strings/string_util.h
File base/strings/string_util.h (right):

https://codereview.chromium.org/543043002/diff/1/base/strings/string_util.h#n...
base/strings/string_util.h:254: BASE_EXPORT bool IsStringASCII(const char* src,
size_t src_len);
On 2014/09/16 17:14:28, brettw wrote:
> The string piece version should cover this case. Actually, I think we should
> have a a StringPiece and a StringPiece16 version of this function and nothing
> else. StringPiece is supposed to handle exactly what you're doing.

You are right, I didn't think about this, thanks!

Overloaded version taking StringPiece16 is useful for testing various input
alignments, so I added it. However, removing the string16 version will require
changing all the places where IsStringASCII is called on a WebString.

WebString provides conversions to string / string16, but not to a StringPiece,
which makes sense, because StringPiece doesn't own the data. I guess, this is
the reason why UTF16To... functions are also operating on string16, and not on
StringPiece16.

I added a comment justifying the existence of the string16 version.

[mnaganov (inactive), 2014-09-22 09:38:16]

mnaganov@chromium.org changed reviewers:
+ thakis@chomium.org
- brettw@chromium.org

[mnaganov (inactive), 2014-09-22 09:38:17]

[+thakis, brettw to CC]

Nico, can you please take a look at this, as Brett seems to be super-busy with
pushing GN.

[Jason Robbins -- corp, 2014-09-24 19:10:50]

jrobbins@google.com changed reviewers:
+ thakis@chromium.org
- thakis@chomium.org

[mnaganov (inactive), 2014-09-26 08:02:15]

mnaganov@chromium.org changed reviewers:
+ darin@chromium.org
- thakis@chromium.org

[mnaganov (inactive), 2014-09-26 08:04:49]

[+darin, thakis to CC]

Now, Nico is OOO. The situation is starting to look desperate.

Darin, can you please, please take a look at this patch? It seems to be
unreasonably hard to get an approval for base/.

[darin (slow to review), 2014-09-26 17:45:11]

darin@chromium.org changed reviewers:
+ brettw@chromium.org

[darin (slow to review), 2014-09-26 17:45:13]

Brett is the better reviewer for string utils.

[brettw, 2014-09-26 17:47:26]

lgtm

[mnaganov (inactive), 2014-09-29 09:28:40]

Thanks, everyone!

[mnaganov (inactive), 2014-09-29 09:29:13]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-09-29 09:30:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/543043002/40001

[commit-bot: I haz the power, 2014-09-29 09:37:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-29 09:37:54]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mnaganov (inactive), 2014-09-29 12:03:04]

Committed patchset #3 (id:40001) manually as
6f500092b9396517a40fdf1ca6a5199738e673e9 (presubmit successful).

[commit-bot: I haz the power, 2014-09-29 12:03:06]

Patchset 3 (id:??) landed as
https://crrev.com/6f500092b9396517a40fdf1ca6a5199738e673e9
Cr-Commit-Position: refs/heads/master@{#297158}