Cache contains only ascii in StringImpl

Cache contains only ascii in StringImpl

This patch adds a lazily calculated value in StringImpl for whether the
string contains only ascii characters. This avoids code that re-scans
the same string many times.

BUG=675493
Committed: https://crrev.com/36ecc9ec6ab699d9300f50fd4ca26030acad3e26
Cr-Commit-Position: refs/heads/master@{#441535}

[Charlie Harrison, 2016-12-18 00:30:44]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-18 00:30:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-18 01:28:11]

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

[commit-bot: I haz the power, 2016-12-18 01:28:12]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Charlie Harrison, 2016-12-18 03:10:18]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-18 03:10:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-18 03:50:08]

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

[commit-bot: I haz the power, 2016-12-18 03:50:08]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Charlie Harrison, 2016-12-18 05:34:47]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-18 05:34:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-18 06:56:06]

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

[commit-bot: I haz the power, 2016-12-18 06:56:07]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Charlie Harrison, 2016-12-19 01:12:04]

Description was changed from

==========
Cache contains only ascii in StringImpl

This patch adds a lazily calculated value in StringImpl for whether the
string contains only ascii characters. This avoids code that re-scans
the same string many times.

BUG=
==========

to

==========
Cache contains only ascii in StringImpl

This patch adds a lazily calculated value in StringImpl for whether the
string contains only ascii characters. This avoids code that re-scans
the same string many times.

BUG=675493
==========

[Charlie Harrison, 2016-12-22 14:24:14]

csharrison@chromium.org changed reviewers:
+ esprehn@chromium.org, kinuko@chromium.org

[Charlie Harrison, 2016-12-22 14:24:14]

esprehn, kinuko: here's the patch to cache containsOnlyASCII at StringImpl
layer.

There's some funkiness with type coercion from bool <-> enum, let me know if
that bothers you and I can fix it up. I mainly did that to avoid
conditionals/ternaries/switches.

Also, there are a number of places where we might know this value ahead of time
(i.e. substring, StringView::toString). I'll follow up in another CL, as I
haven't found the best way to shape the API around that.

[kinuko, 2016-12-26 07:32:19]

https://codereview.chromium.org/2585063002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/StringImpl.h (right):

https://codereview.chromium.org/2585063002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:554: :
charactersAreAllASCII(characters16(), length()));
I'd defer the judgement to esprehn (so you might want to hold off) but I'm not a
big fan of this enum <-> boolean conversions (assuming we wouldn't get any perf
benefit after optimization runs).  Would clearly writing down the switches or
ternaries make the code that bigger?

[esprehn, 2016-12-26 19:33:56]

Since you're using two bits anyway you can use one bit for m_needsAsciiCheck and
another for m_containsOnlyAscii with the first defaulted to true and the second
to false.

[Charlie Harrison, 2017-01-03 16:28:43]

On 2016/12/26 19:33:56, esprehn wrote:
> Since you're using two bits anyway you can use one bit for m_needsAsciiCheck
and
> another for m_containsOnlyAscii with the first defaulted to true and the
second
> to false.

After thinking about it for a bit, I have decided to go this route. PTAL?

[esprehn, 2017-01-04 20:30:38]

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/StringImpl.h (right):

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:137: m_containsOnlyASCII(false),
true

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:138: m_needsASCIICheck(true),
false

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:155: m_containsOnlyASCII(false),
m_containsOnlyASCII(true), the string is empty

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:156: m_needsASCIICheck(true),
false

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:170: m_containsOnlyASCII(false),
(!length)

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:171: m_needsASCIICheck(true),
(length)

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:183: m_containsOnlyASCII(false),
(!length)

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:184: m_needsASCIICheck(true),
(length)

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:198: m_needsASCIICheck(true),
ditto

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:548: !length() ||
Strings are immutable, so you don't need the length check here, you can do it in
the constructor.

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:550: :
charactersAreAllASCII(characters16(), length()));
This is inlining a lot of code which I know the old code did, but  instead I
think you want to put the slow path in a separate out of line method, and only
ALWAYS_INLINE the fast path. That'll actually be faster since it makes the
functions smaller.

ALWAYS_INLINE bool StringImpl::containsOnlyASCII() const {
  if (m_needsASCIICheck)
    updateContainsOnlyASCII();
  return m_containsOnlyASCII;
}

and updateContainsOnlyASCII is in the .cpp file out of line.

[Charlie Harrison, 2017-01-04 20:53:20]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 20:53:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-01-04 20:53:51]

Thanks!

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/StringImpl.h (right):

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:137: m_containsOnlyASCII(false),
On 2017/01/04 20:30:37, esprehn wrote:
> true

Done.

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:138: m_needsASCIICheck(true),
On 2017/01/04 20:30:37, esprehn wrote:
> false

Done.

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:155: m_containsOnlyASCII(false),
On 2017/01/04 20:30:37, esprehn wrote:
> m_containsOnlyASCII(true), the string is empty

Done.

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:156: m_needsASCIICheck(true),
On 2017/01/04 20:30:37, esprehn wrote:
> false

Done.

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:170: m_containsOnlyASCII(false),
On 2017/01/04 20:30:37, esprehn wrote:
> (!length)

Done.

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:171: m_needsASCIICheck(true),
On 2017/01/04 20:30:37, esprehn wrote:
> (length)

Done.

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:183: m_containsOnlyASCII(false),
On 2017/01/04 20:30:37, esprehn wrote:
> (!length)

Done.

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:184: m_needsASCIICheck(true),
On 2017/01/04 20:30:37, esprehn wrote:
> (length)

Done.

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:198: m_needsASCIICheck(true),
On 2017/01/04 20:30:37, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:548: !length() ||
On 2017/01/04 20:30:37, esprehn wrote:
> Strings are immutable, so you don't need the length check here, you can do it
in
> the constructor.

Done.

https://codereview.chromium.org/2585063002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.h:550: :
charactersAreAllASCII(characters16(), length()));
On 2017/01/04 20:30:37, esprehn wrote:
> This is inlining a lot of code which I know the old code did, but  instead I
> think you want to put the slow path in a separate out of line method, and only
> ALWAYS_INLINE the fast path. That'll actually be faster since it makes the
> functions smaller.
> 
> ALWAYS_INLINE bool StringImpl::containsOnlyASCII() const {
>   if (m_needsASCIICheck)
>     updateContainsOnlyASCII();
>   return m_containsOnlyASCII;
> }
> 
> and updateContainsOnlyASCII is in the .cpp file out of line.

SGTM. Done.

[esprehn, 2017-01-04 20:57:50]

lgtm, I think you might want to add an if (!length) return true; check to the
top of charactersAreAllASCII too for safety reasons, looking at that function
I'm not sure it anticipates an empty string.

[esprehn, 2017-01-04 20:58:23]

On 2017/01/04 at 20:57:50, esprehn wrote:
> lgtm, I think you might want to add an if (!length) return true; check to the
top of charactersAreAllASCII too for safety reasons, looking at that function
I'm not sure it anticipates an empty string.

.... or a DCHECK_GT(length, 0) since we handle the length checks in the
constructor now.

[commit-bot: I haz the power, 2017-01-04 21:06:40]

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

[commit-bot: I haz the power, 2017-01-04 21:06:44]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Charlie Harrison, 2017-01-04 21:40:42]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 21:41:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-01-04 21:43:29]

Added the DCHECK. I also had to static_cast<bool>(length) to initialize
m_needsASCIICheck, as m_needsASCIICheck(unsigned) does not do bool coercion.

[Charlie Harrison, 2017-01-04 21:43:39]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-01-04 21:43:39]

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2017-01-04 21:47:00]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-01-04 21:47:00]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2585063002/#ps80001
(title: "DCHECK + static_Cast")

[commit-bot: I haz the power, 2017-01-04 21:47:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 00:52:40]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483566420429600,
"parent_rev": "05b3b665ced9e67a0ca5a6f17b5d8c5b77608aaa", "commit_rev":
"1ceea5ff18c4a5cc983215cc77c507707fc6985b"}

[commit-bot: I haz the power, 2017-01-05 00:53:09]

Description was changed from

==========
Cache contains only ascii in StringImpl

This patch adds a lazily calculated value in StringImpl for whether the
string contains only ascii characters. This avoids code that re-scans
the same string many times.

BUG=675493
==========

to

==========
Cache contains only ascii in StringImpl

This patch adds a lazily calculated value in StringImpl for whether the
string contains only ascii characters. This avoids code that re-scans
the same string many times.

BUG=675493

Review-Url: https://codereview.chromium.org/2585063002
==========

[commit-bot: I haz the power, 2017-01-05 00:53:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2017-01-05 00:56:50]

Description was changed from

==========
Cache contains only ascii in StringImpl

This patch adds a lazily calculated value in StringImpl for whether the
string contains only ascii characters. This avoids code that re-scans
the same string many times.

BUG=675493

Review-Url: https://codereview.chromium.org/2585063002
==========

to

==========
Cache contains only ascii in StringImpl

This patch adds a lazily calculated value in StringImpl for whether the
string contains only ascii characters. This avoids code that re-scans
the same string many times.

BUG=675493

Committed: https://crrev.com/36ecc9ec6ab699d9300f50fd4ca26030acad3e26
Cr-Commit-Position: refs/heads/master@{#441535}
==========

[commit-bot: I haz the power, 2017-01-05 00:56:51]

Patchset 5 (id:??) landed as
https://crrev.com/36ecc9ec6ab699d9300f50fd4ca26030acad3e26
Cr-Commit-Position: refs/heads/master@{#441535}