Remove SegmentedString::m_pushedChar{1,2} optimization

Remove SegmentedString::m_pushedChar{1,2} optimization

Before r201294, SegmentedString::push() had an exotic behavior where two
consecutive push() results are swapped.  SegmentedString stored those two
chars into m_pushedChar{1,2} to avoid messing up SegmentedSubString.

After r201294, SegmentedString::push() is guaranteed to be used for
ungetc()-like usecase, where it reverts the SegmentedString::advance()
which was issued just before the call.

This CL removes m_pushedChar{1,2} optimization, and replace it with
SegmentedSubstring::push() which simply decrement m_data.string{8,16}Ptr.
In pessimistic case, this may end up prepending wtf::String inside
SegmentedSubstring, but this should be very rare.

BUG=None
TESTS=SegmentedStringTest

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201517

[kouhei (in TOK), 2015-08-27 09:50:08]

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

[commit-bot: I haz the power, 2015-08-27 09:50:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1319913002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1319913002/1

[kouhei (in TOK), 2015-08-27 09:50:16]

kouhei@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-08-27 10:48:34]

Is there anyone who is familiar with SegmentedString? (As far as I see the git
history of SegmentedString, it hasn't been touched for a long time...)

What I can do would be just to stamp the CL :-/

[kouhei (in TOK), 2015-08-27 10:55:11]

kouhei@chromium.org changed reviewers:
+ yoav@yoav.ws

[kouhei (in TOK), 2015-08-27 10:55:11]

On 2015/08/27 10:48:34, haraken wrote:
> Is there anyone who is familiar with SegmentedString? (As far as I see the git
> history of SegmentedString, it hasn't been touched for a long time...)
> 
> What I can do would be just to stamp the CL :-/

Yeah, this part of the code was not maintained for a long time despite its
importance.
Adding in yoav@ as we believe this is a part of parser code.

[kouhei (in TOK), 2015-08-27 10:57:18]

It looks like this is causing crash in some LayoutTests. I'll fix them tomorrow.
Please hold on review.

[commit-bot: I haz the power, 2015-08-27 11:09:50]

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

[commit-bot: I haz the power, 2015-08-27 11:09:51]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yoav Weiss, 2015-08-27 11:13:57]

On 2015/08/27 10:55:11, kouhei wrote:
> On 2015/08/27 10:48:34, haraken wrote:
> > Is there anyone who is familiar with SegmentedString? (As far as I see the
git
> > history of SegmentedString, it hasn't been touched for a long time...)
> > 
> > What I can do would be just to stamp the CL :-/
> 
> Yeah, this part of the code was not maintained for a long time despite its
> importance.
> Adding in yoav@ as we believe this is a part of parser code.

I'm not highly familiar with SegmentedString, but I can study it to understand
this change and review it.
Can you provide links to CLs that changed the calling behavior, making this
optimization unnecessary?

[kouhei (in TOK), 2015-08-27 11:21:03]

On 2015/08/27 11:13:57, Yoav Weiss wrote:
> On 2015/08/27 10:55:11, kouhei wrote:
> > On 2015/08/27 10:48:34, haraken wrote:
> > > Is there anyone who is familiar with SegmentedString? (As far as I see the
> git
> > > history of SegmentedString, it hasn't been touched for a long time...)
> > > 
> > > What I can do would be just to stamp the CL :-/
> > 
> > Yeah, this part of the code was not maintained for a long time despite its
> > importance.
> > Adding in yoav@ as we believe this is a part of parser code.
> 
> I'm not highly familiar with SegmentedString, but I can study it to understand
> this change and review it.
I deeply appreciate it. I don't think anyone currently active is familiar with
this part of the code.
Historically, abarth@ and eseidel@ used to maintain this and other parser
classes.

> Can you provide links to CLs that changed the calling behavior, making this
> optimization unnecessary?
Here it is: https://codereview.chromium.org/1308573006/

[Yoav Weiss, 2015-08-27 12:59:34]

On 2015/08/27 11:21:03, kouhei wrote:
> On 2015/08/27 11:13:57, Yoav Weiss wrote:
> > On 2015/08/27 10:55:11, kouhei wrote:
> > > On 2015/08/27 10:48:34, haraken wrote:
> > > > Is there anyone who is familiar with SegmentedString? (As far as I see
the
> > git
> > > > history of SegmentedString, it hasn't been touched for a long time...)
> > > > 
> > > > What I can do would be just to stamp the CL :-/
> > > 
> > > Yeah, this part of the code was not maintained for a long time despite its
> > > importance.
> > > Adding in yoav@ as we believe this is a part of parser code.
> > 
> > I'm not highly familiar with SegmentedString, but I can study it to
understand
> > this change and review it.
> I deeply appreciate it. I don't think anyone currently active is familiar with
> this part of the code.
> Historically, abarth@ and eseidel@ used to maintain this and other parser
> classes.
> 
> > Can you provide links to CLs that changed the calling behavior, making this
> > optimization unnecessary?
> Here it is: https://codereview.chromium.org/1308573006/

Thanks! Out of curiosity, do you know the historical reason for the weird push
behavior?

[kouhei (in TOK), 2015-08-27 13:03:20]

> > > Can you provide links to CLs that changed the calling behavior, making
this
> > > optimization unnecessary?
> > Here it is: https://codereview.chromium.org/1308573006/
> 
> Thanks! Out of curiosity, do you know the historical reason for the weird push
> behavior?

Unfortunately, no. :/

This work is a part of yak shaving toward fixing
https://code.google.com/p/chromium/issues/detail?id=520296 bug.
I want to have SegmentedString support multiple readers to avoid cloning in
HTMLSourceTracker, but the class obviously needs refactoring first. The current
code is full of wonders :p

[Yoav Weiss, 2015-08-27 13:08:12]

https://codereview.chromium.org/1319913002/diff/1/Source/platform/text/Segmen...
File Source/platform/text/SegmentedString.h (right):

https://codereview.chromium.org/1319913002/diff/1/Source/platform/text/Segmen...
Source/platform/text/SegmentedString.h:54: m_data.string16Ptr =
m_string.characters16();
Unrelated to current CL, but when str.length() is zero, m_data is not
initialized!

[Yoav Weiss, 2015-08-27 13:19:04]

https://codereview.chromium.org/1319913002/diff/1/Source/platform/text/Segmen...
File Source/platform/text/SegmentedString.h (right):

https://codereview.chromium.org/1319913002/diff/1/Source/platform/text/Segmen...
Source/platform/text/SegmentedString.h:88: m_string.insert(&c, 1, 0);
AFAICT this goes back to String::append which looks pretty awful in terms of
performance :/
Can we somehow use a StringBuilder for that?

[kouhei (in TOK), 2015-08-28 05:28:11]

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

[commit-bot: I haz the power, 2015-08-28 05:28:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1319913002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1319913002/20001

[kouhei (in TOK), 2015-08-28 05:38:47]

https://codereview.chromium.org/1319913002/diff/1/Source/platform/text/Segmen...
File Source/platform/text/SegmentedString.h (right):

https://codereview.chromium.org/1319913002/diff/1/Source/platform/text/Segmen...
Source/platform/text/SegmentedString.h:54: m_data.string16Ptr =
m_string.characters16();
On 2015/08/27 13:08:12, Yoav Weiss wrote:
> Unrelated to current CL, but when str.length() is zero, m_data is not
> initialized!

Done.

https://codereview.chromium.org/1319913002/diff/1/Source/platform/text/Segmen...
Source/platform/text/SegmentedString.h:88: m_string.insert(&c, 1, 0);
On 2015/08/27 13:19:03, Yoav Weiss wrote:
> AFAICT this goes back to String::append which looks pretty awful in terms of
> performance :/
> Can we somehow use a StringBuilder for that?

I replaced this codepath with prepend() call in SegmentedString::push(). Not
optimal, but better than insert() call.

SegmentedString::push() is only used for undo-ing advance(), so chances are that
--m_data.string{8,16}Ptr path is taken almost all of the time. I have never seen
this codepath being taken except for some unittests/LayoutTests.

[commit-bot: I haz the power, 2015-08-28 07:41:07]

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

[commit-bot: I haz the power, 2015-08-28 07:41:08]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2015-08-31 04:10:32]

yoav: Would you take a look?

[Yoav Weiss, 2015-08-31 07:57:44]

LGTM % nit.

Thanks for making this class saner! :)

https://codereview.chromium.org/1319913002/diff/20001/Source/platform/text/Se...
File Source/platform/text/SegmentedString.h (right):

https://codereview.chromium.org/1319913002/diff/20001/Source/platform/text/Se...
Source/platform/text/SegmentedString.h:294: bool escaped() const { return false;
}
I believe we can get rid of escaped() altogether.

[kouhei (in TOK), 2015-09-01 00:07:21]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2015-09-01 00:07:21]

The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws
Link to the patchset: https://codereview.chromium.org/1319913002/#ps40001
(title: "remove escaped()")

[commit-bot: I haz the power, 2015-09-01 00:07:34]

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

[kouhei (in TOK), 2015-09-01 00:16:33]

haraken: Would you OWNER approve platform/text change?

[commit-bot: I haz the power, 2015-09-01 00:16:34]

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

[commit-bot: I haz the power, 2015-09-01 00:16:35]

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

[kouhei (in TOK), 2015-09-01 00:17:06]

Thanks for review!

>
https://codereview.chromium.org/1319913002/diff/20001/Source/platform/text/Se...
> File Source/platform/text/SegmentedString.h (right):
> 
>
https://codereview.chromium.org/1319913002/diff/20001/Source/platform/text/Se...
> Source/platform/text/SegmentedString.h:294: bool escaped() const { return
false;
> }
> I believe we can get rid of escaped() altogether.

Done.

[haraken, 2015-09-01 00:22:07]

rubberstamp LGTM as an owner.

[kouhei (in TOK), 2015-09-01 01:57:27]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2015-09-01 01:57:32]

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

[commit-bot: I haz the power, 2015-09-01 03:45:25]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201517