Internalize strings in-place

Internalize strings in-place (reland^3)

using newly introduced ThinStrings, which store a pointer to the actual,
internalized string they represent.

BUG=v8:4520
(Previously landed as #42168 / af51befe694f)
(Previously landed as #42193 / 4c699e349a49)
(Previously landed as #42235 / ec45e6ed2e11)

Review-Url: https://codereview.chromium.org/2549773002
Cr-Commit-Position: refs/heads/master@{#42503}
Committed: https://chromium.googlesource.com/v8/v8/+/3e915e12a146fb346765ab5027c5e9ebad0600f9

[Jakob Kummerow, 2016-12-16 23:12:18]

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

[commit-bot: I haz the power, 2016-12-16 23:12:32]

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-16 23:16:16]

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

[commit-bot: I haz the power, 2016-12-16 23:16:17]

Dry run: Try jobs failed on following builders:
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

[Jakob Kummerow, 2016-12-17 02:33:50]

jkummerow@chromium.org changed reviewers:
+ bmeurer@chromium.org, hpayer@chromium.org, ishell@chromium.org

[Jakob Kummerow, 2016-12-17 02:33:51]

Igor: PTAL.

Benedikt: Please review src/compiler/. This is my first nontrivial TF patch, so
please review carefully if I've done the value/effect/control wiring correctly.

Hannes: Please review src/heap/. I'm aware that adding additional behavior to
the Scavenger is generally undesirable, but this seemed simple enough? I'm also
aware that the Scavenger is going away and the MinorMC doesn't support
shortcutting (at least for now?); that's fine, as it's just a performance
optimization, not required for correctness. I can drop that code if you prefer.
(From a GC point of view, a ThinString is semantically similar to a forwarding
pointer: slots pointing to it should ideally be updated to point to the
referenced actual string directly instead.)

MIPS/PPC/S390 teams: Please review the respective platform-specific changes.

Yang: FYI.

I'm happy to change the naming if anyone has better suggestions than "thin"
string and "actual" value.

[Jakob Kummerow, 2016-12-17 02:34:03]

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

[commit-bot: I haz the power, 2016-12-17 02:34:11]

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-17 02:40:00]

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

[commit-bot: I haz the power, 2016-12-17 02:40:01]

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

[Jakob Kummerow, 2016-12-17 03:05:54]

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

[commit-bot: I haz the power, 2016-12-17 03:05: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-17 03:18:06]

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

[commit-bot: I haz the power, 2016-12-17 03:18:07]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_nodcheck_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng_tr...)

[Benedikt Meurer, 2016-12-17 11:42:29]

LGTM on compiler!

[Yang, 2016-12-19 14:15:27]

On 2016/12/17 11:42:29, Benedikt Meurer wrote:
> LGTM on compiler!

src/regexp lgtm.

[Jakob Kummerow, 2016-12-19 18:15:40]

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

[commit-bot: I haz the power, 2016-12-19 18:15:44]

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-19 18:17:34]

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

[commit-bot: I haz the power, 2016-12-19 18:17:34]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/18096)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/18156)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/18062)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/19722)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/19800)

[Igor Sheludko, 2016-12-20 23:46:58]

OMG! Awesome!

"actual" could probably be "internalized". 

lgtm with nits:

https://codereview.chromium.org/2549773002/diff/240001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2549773002/diff/240001/src/code-stub-assemble...
src/code-stub-assembler.cc:3381: // The subject string is either a sliced or
cons string.
or thin string.

https://codereview.chromium.org/2549773002/diff/240001/src/code-stub-assemble...
src/code-stub-assembler.cc:3405: GotoIf(Word32Equal(Word32And(instance_type,
Maybe reuse the result of "Word32And(instance_type, ...)" from line 3386.

https://codereview.chromium.org/2549773002/diff/240001/src/compiler/effect-co...
File src/compiler/effect-control-linearizer.cc (right):

https://codereview.chromium.org/2549773002/diff/240001/src/compiler/effect-co...
src/compiler/effect-control-linearizer.cc:2234: // We may need to loop several
times for ConsString/SlicedString {subject}s.
/ThinString

https://codereview.chromium.org/2549773002/diff/240001/src/ia32/code-stubs-ia...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/2549773002/diff/240001/src/ia32/code-stubs-ia...
src/ia32/code-stubs-ia32.cc:636: STATIC_ASSERT(kSlicedStringTag >
kExternalStringTag);
Maybe
  STATIC_ASSERT(kThinStringTag > kExternalStringTag);
here and in other ports?

https://codereview.chromium.org/2549773002/diff/240001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/2549773002/diff/240001/src/objects-debug.cc#n...
src/objects-debug.cc:555: }
} else if (IsThinString()) {
  ThinString::cast(this)->ThinStringVerify();
}

Please verify that the actual is an internalized string.

https://codereview.chromium.org/2549773002/diff/240001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2549773002/diff/240001/src/objects.cc#newcode...
src/objects.cc:2335: this->IsConsString() || this->IsSlicedString() ||
this->IsThinString();
Suggestion for follow-up CL: 
bool IsIndirectString() {
  return (map()->instance_type() & kIsIndirectStringMask) ==
kIsIndirectStringTag;
}

https://codereview.chromium.org/2549773002/diff/240001/src/objects.cc#newcode...
src/objects.cc:2408: this->IsConsString() || this->IsSlicedString() ||
this->IsThinString();
Same here.

https://codereview.chromium.org/2549773002/diff/240001/src/objects.cc#newcode...
src/objects.cc:17863: Heap* heap = isolate->heap();
Maybe wrap below lines in "if (size_delta) {" to simplify small-string case
(because AdjustLiveBytes() looks heavy).

[Hannes Payer (out of office), 2016-12-21 09:53:14]

https://codereview.chromium.org/2549773002/diff/240001/src/heap/scavenger.cc
File src/heap/scavenger.cc (right):

https://codereview.chromium.org/2549773002/diff/240001/src/heap/scavenger.cc#...
src/heap/scavenger.cc:445: scavenging_visitors_table_.Register(
Instead of using a special visitor when compacting, we could just record the
slot when updating the slot with the actual string (above).

https://codereview.chromium.org/2549773002/diff/240001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2549773002/diff/240001/src/objects.h#newcode1...
src/objects.h:10317: class ThinString : public String {
Please add a description what a ThinString is.

[Jakob Kummerow, 2017-01-04 12:45:06]

Addressed all comments but one.

Hannes, can you please clarify your idea for scavenger.cc? (We can discuss on
Monday.)

https://codereview.chromium.org/2549773002/diff/240001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2549773002/diff/240001/src/code-stub-assemble...
src/code-stub-assembler.cc:3381: // The subject string is either a sliced or
cons string.
On 2016/12/20 23:46:58, Igor Sheludko wrote:
> or thin string.

Done.

https://codereview.chromium.org/2549773002/diff/240001/src/code-stub-assemble...
src/code-stub-assembler.cc:3405: GotoIf(Word32Equal(Word32And(instance_type,
On 2016/12/20 23:46:58, Igor Sheludko wrote:
> Maybe reuse the result of "Word32And(instance_type, ...)" from line 3386.

Done.

https://codereview.chromium.org/2549773002/diff/240001/src/compiler/effect-co...
File src/compiler/effect-control-linearizer.cc (right):

https://codereview.chromium.org/2549773002/diff/240001/src/compiler/effect-co...
src/compiler/effect-control-linearizer.cc:2234: // We may need to loop several
times for ConsString/SlicedString {subject}s.
On 2016/12/20 23:46:58, Igor Sheludko wrote:
> /ThinString

Done.

https://codereview.chromium.org/2549773002/diff/240001/src/heap/scavenger.cc
File src/heap/scavenger.cc (right):

https://codereview.chromium.org/2549773002/diff/240001/src/heap/scavenger.cc#...
src/heap/scavenger.cc:445: scavenging_visitors_table_.Register(
On 2016/12/21 09:53:14, Hannes Payer wrote:
> Instead of using a special visitor when compacting, we could just record the
> slot when updating the slot with the actual string (above).

I don't understand what you mean here, can you please clarify?
(I realize that the patch as-is doesn't make a lot of sense, as in particular
the ConsString corner case being addressed here doesn't apply to ThinStrings,
but I don't understand what alternative precisely you have in mind.)

https://codereview.chromium.org/2549773002/diff/240001/src/ia32/code-stubs-ia...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/2549773002/diff/240001/src/ia32/code-stubs-ia...
src/ia32/code-stubs-ia32.cc:636: STATIC_ASSERT(kSlicedStringTag >
kExternalStringTag);
On 2016/12/20 23:46:58, Igor Sheludko wrote:
> Maybe
>   STATIC_ASSERT(kThinStringTag > kExternalStringTag);
> here and in other ports?

Done.

https://codereview.chromium.org/2549773002/diff/240001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/2549773002/diff/240001/src/objects-debug.cc#n...
src/objects-debug.cc:555: }
On 2016/12/20 23:46:58, Igor Sheludko wrote:
> } else if (IsThinString()) {
>   ThinString::cast(this)->ThinStringVerify();
> }
> 
> Please verify that the actual is an internalized string.

Done.

https://codereview.chromium.org/2549773002/diff/240001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2549773002/diff/240001/src/objects.cc#newcode...
src/objects.cc:2335: this->IsConsString() || this->IsSlicedString() ||
this->IsThinString();
On 2016/12/20 23:46:58, Igor Sheludko wrote:
> Suggestion for follow-up CL: 
> bool IsIndirectString() {
>   return (map()->instance_type() & kIsIndirectStringMask) ==
> kIsIndirectStringTag;
> }

Done (StringShape supports this already).

https://codereview.chromium.org/2549773002/diff/240001/src/objects.cc#newcode...
src/objects.cc:2408: this->IsConsString() || this->IsSlicedString() ||
this->IsThinString();
On 2016/12/20 23:46:58, Igor Sheludko wrote:
> Same here.

Done.

https://codereview.chromium.org/2549773002/diff/240001/src/objects.cc#newcode...
src/objects.cc:17863: Heap* heap = isolate->heap();
On 2016/12/20 23:46:58, Igor Sheludko wrote:
> Maybe wrap below lines in "if (size_delta) {" to simplify small-string case
> (because AdjustLiveBytes() looks heavy).

Done.

https://codereview.chromium.org/2549773002/diff/240001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2549773002/diff/240001/src/objects.h#newcode1...
src/objects.h:10317: class ThinString : public String {
On 2016/12/21 09:53:14, Hannes Payer wrote:
> Please add a description what a ThinString is.

Done.

[Jakob Kummerow, 2017-01-09 17:42:31]

Hannes, can you approve this please?

https://codereview.chromium.org/2549773002/diff/240001/src/heap/scavenger.cc
File src/heap/scavenger.cc (right):

https://codereview.chromium.org/2549773002/diff/240001/src/heap/scavenger.cc#...
src/heap/scavenger.cc:445: scavenging_visitors_table_.Register(
On 2016/12/21 09:53:14, Hannes Payer wrote:
> Instead of using a special visitor when compacting, we could just record the
> slot when updating the slot with the actual string (above).

As discussed offline, this appears to be much harder than it sounds, so since
the Scavenger will likely get replaced by the MinorMC anyway, we're not going to
do it for now.

[Jakob Kummerow, 2017-01-09 17:42:40]

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

[commit-bot: I haz the power, 2017-01-09 17:42:49]

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, 2017-01-09 18:18:14]

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

[commit-bot: I haz the power, 2017-01-09 18:18:15]

Dry run: This issue passed the CQ dry run.

[Hannes Payer (out of office), 2017-01-10 10:19:20]

lgtm

[Jakob Kummerow, 2017-01-10 10:20:58]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2017-01-10 10:20:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, yangguo@chromium.org, ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2549773002/#ps300001
(title: "rebased")

[commit-bot: I haz the power, 2017-01-10 10:21:02]

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-10 10:58:05]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1484043658752690,
"parent_rev": "d1f347fa54ad962c1d109be0e194ff85784137da", "commit_rev":
"af51befe694fe039db3554d4b9165f7d6baceb77"}

[commit-bot: I haz the power, 2017-01-10 10:58:11]

Description was changed from

==========
Internalize strings in-place

using newly introduced ThinStrings, which store a pointer to the actual,
internalized string they represent.

BUG=v8:4520
==========

to

==========
Internalize strings in-place

using newly introduced ThinStrings, which store a pointer to the actual,
internalized string they represent.

BUG=v8:4520

Review-Url: https://codereview.chromium.org/2549773002
Cr-Commit-Position: refs/heads/master@{#42168}
Committed:
https://chromium.googlesource.com/v8/v8/+/af51befe694fe039db3554d4b9165f7d6ba...
==========

[commit-bot: I haz the power, 2017-01-10 10:58:12]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/v8/v8/+/af51befe694fe039db3554d4b9165f7d6ba...

[Michael Achenbach, 2017-01-10 11:32:39]

A revert of this CL (patchset #16 id:300001) has been created in
https://codereview.chromium.org/2621913002/ by machenbach@chromium.org.

The reason for reverting is: gc stress failures:
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20gc%20stress/....

[Michael Achenbach, 2017-01-10 11:55:37]

On 2017/01/10 11:32:39, Michael Achenbach wrote:
> A revert of this CL (patchset #16 id:300001) has been created in
> https://codereview.chromium.org/2621913002/ by mailto:machenbach@chromium.org.
> 
> The reason for reverting is: gc stress failures:
>
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20gc%20stress/....

[Michael Achenbach, 2017-01-10 11:56:01]

Also breaks layout tests:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui...

[Jakob Kummerow, 2017-01-10 15:51:03]

Turns out both SlicedStrings and flat ConsStrings can point to ThinStrings as
their parent()/first(); this happens when their original parent()/first() is
internalized in-place. Flattening operations must handle these cases.

CSA::FlattenString() was unused.

Please review the diff from patch set 16 -> 17.

[Jakob Kummerow, 2017-01-10 15:52:47]

Description was changed from

==========
Internalize strings in-place

using newly introduced ThinStrings, which store a pointer to the actual,
internalized string they represent.

BUG=v8:4520

Review-Url: https://codereview.chromium.org/2549773002
Cr-Commit-Position: refs/heads/master@{#42168}
Committed:
https://chromium.googlesource.com/v8/v8/+/af51befe694fe039db3554d4b9165f7d6ba...
==========

to

==========
Internalize strings in-place (reland)

using newly introduced ThinStrings, which store a pointer to the actual,
internalized string they represent.

BUG=v8:4520

(Previously landed as #42168 / af51befe694f.
==========

[Igor Sheludko, 2017-01-10 16:03:34]

lgtm

[Jakob Kummerow, 2017-01-10 16:07:56]

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

[commit-bot: I haz the power, 2017-01-10 16:08:07]

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, 2017-01-10 16:37:54]

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

[commit-bot: I haz the power, 2017-01-10 16:37:56]

Dry run: This issue passed the CQ dry run.

[Jakob Kummerow, 2017-01-10 19:06:11]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2017-01-10 19:06:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, hpayer@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2549773002/#ps320001
(title: "fixes for reland (for cons+sliced strings)")

[commit-bot: I haz the power, 2017-01-10 19:06:20]

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-10 19:08:59]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1484075171291210,
"parent_rev": "0c4b8ff44ce4ef0cae06c964679c98c32f8bd40f", "commit_rev":
"4c699e349a4986b28574b3a51e8780e3a3d067b1"}

[commit-bot: I haz the power, 2017-01-10 19:09:04]

Description was changed from

==========
Internalize strings in-place (reland)

using newly introduced ThinStrings, which store a pointer to the actual,
internalized string they represent.

BUG=v8:4520

(Previously landed as #42168 / af51befe694f.
==========

to

==========
Internalize strings in-place (reland)

using newly introduced ThinStrings, which store a pointer to the actual,
internalized string they represent.

BUG=v8:4520

(Previously landed as #42168 / af51befe694f.

Review-Url: https://codereview.chromium.org/2549773002
Cr-Commit-Position: refs/heads/master@{#42193}
Committed:
https://chromium.googlesource.com/v8/v8/+/4c699e349a4986b28574b3a51e8780e3a3d...
==========

[commit-bot: I haz the power, 2017-01-10 19:09:05]

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/v8/v8/+/4c699e349a4986b28574b3a51e8780e3a3d...

[Jakob Kummerow, 2017-01-11 09:59:29]

A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/2625073002/ by jkummerow@chromium.org.

The reason for reverting is: blocks roll, see:
https://codereview.chromium.org/2628733002/

Debug mode runs into an Abort("External string expected, but not found")..

[Jakob Kummerow, 2017-01-11 11:55:52]

Description was changed from

==========
Internalize strings in-place (reland)

using newly introduced ThinStrings, which store a pointer to the actual,
internalized string they represent.

BUG=v8:4520

(Previously landed as #42168 / af51befe694f.

Review-Url: https://codereview.chromium.org/2549773002
Cr-Commit-Position: refs/heads/master@{#42193}
Committed:
https://chromium.googlesource.com/v8/v8/+/4c699e349a4986b28574b3a51e8780e3a3d...
==========

to

==========
Internalize strings in-place (reland^2)

using newly introduced ThinStrings, which store a pointer to the actual,
internalized string they represent.

BUG=v8:4520

(Previously landed as #42168 / af51befe694f)
(Previously landed as #42193 / 4c699e349a49)
==========

[Jakob Kummerow, 2017-01-11 12:03:46]

Of course if you fix something in C++, you also have to fix it in handwritten
assembly...

PTAL at patch set 17 -> 18.

[Igor Sheludko, 2017-01-11 12:10:38]

lgtm

[Jakob Kummerow, 2017-01-11 13:18:15]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2017-01-11 13:18:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, hpayer@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2549773002/#ps340001
(title: "fixes for 2nd reland")

[commit-bot: I haz the power, 2017-01-11 13:18:29]

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-11 13:37:43]

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

[commit-bot: I haz the power, 2017-01-11 13:37:45]

Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[Jakob Kummerow, 2017-01-11 14:19:18]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2017-01-11 14:19:19]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org,
bmeurer@chromium.org, hpayer@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2549773002/#ps380001
(title: "rebased")

[commit-bot: I haz the power, 2017-01-11 14:19:33]

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-11 14:59:36]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1484144358732960,
"parent_rev": "0befccd21b2634b841bcec11c51743c50d169796", "commit_rev":
"ec45e6ed2e11698c713e664b1510bc31bcdbbdba"}

[commit-bot: I haz the power, 2017-01-11 14:59:41]

Description was changed from

==========
Internalize strings in-place (reland^2)

using newly introduced ThinStrings, which store a pointer to the actual,
internalized string they represent.

BUG=v8:4520

(Previously landed as #42168 / af51befe694f)
(Previously landed as #42193 / 4c699e349a49)
==========

to

==========
Internalize strings in-place (reland^2)

using newly introduced ThinStrings, which store a pointer to the actual,
internalized string they represent.

BUG=v8:4520

(Previously landed as #42168 / af51befe694f)
(Previously landed as #42193 / 4c699e349a49)

Review-Url: https://codereview.chromium.org/2549773002
Cr-Commit-Position: refs/heads/master@{#42235}
Committed:
https://chromium.googlesource.com/v8/v8/+/ec45e6ed2e11698c713e664b1510bc31bcd...
==========

[commit-bot: I haz the power, 2017-01-11 14:59:43]

Committed patchset #20 (id:380001) as
https://chromium.googlesource.com/v8/v8/+/ec45e6ed2e11698c713e664b1510bc31bcd...

[Jakob Kummerow, 2017-01-12 14:49:39]

A revert of this CL (patchset #20 id:380001) has been created in
https://codereview.chromium.org/2626893005/ by jkummerow@chromium.org.

The reason for reverting is: Blocks roll, ASan detects leaking ExternalStrings..

[Jakob Kummerow, 2017-01-18 13:26:50]

Description was changed from

==========
Internalize strings in-place (reland^2)

using newly introduced ThinStrings, which store a pointer to the actual,
internalized string they represent.

BUG=v8:4520

(Previously landed as #42168 / af51befe694f)
(Previously landed as #42193 / 4c699e349a49)

Review-Url: https://codereview.chromium.org/2549773002
Cr-Commit-Position: refs/heads/master@{#42235}
Committed:
https://chromium.googlesource.com/v8/v8/+/ec45e6ed2e11698c713e664b1510bc31bcd...
==========

to

==========
Internalize strings in-place (reland^3)

using newly introduced ThinStrings, which store a pointer to the actual,
internalized string they represent.

BUG=v8:4520

(Previously landed as #42168 / af51befe694f)
(Previously landed as #42193 / 4c699e349a49)
(Previously landed as #42235 / ec45e6ed2e11)
==========

[Jakob Kummerow, 2017-01-18 14:08:17]

Ready for another look.

Patch set 21 is just a revert of the revert, as a basis for reviews.

Patch set 22 includes the mips/mips64/ppc/s390 fixes from
https://codereview.chromium.org/2628643002/ and
https://codereview.chromium.org/2625933006/.

Patch sets 23 and 24 are small fixes for LargeObjectSpace verification and an
external string resource leak that ASan found, respectively.

Patch set 25 fixes performance regressions by making the TF builtins for
StringAdd, StringEqual, StringRelationalComparison aware of ThinStrings, thereby
avoiding runtime calls.

[Jakob Kummerow, 2017-01-18 14:11:55]

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

[commit-bot: I haz the power, 2017-01-18 14:12:00]

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, 2017-01-18 14:50:22]

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

[commit-bot: I haz the power, 2017-01-18 14:50:24]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2017-01-18 14:55:21]

lgtm

https://codereview.chromium.org/2549773002/diff/480001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2549773002/diff/480001/src/code-stub-assemble...
src/code-stub-assembler.cc:3583: GotoIf(WordEqual(rhs, EmptyStringConstant()),
&deref);
Maybe we should stop using ConsStrings this way once ThinString sticks in the
tree.

https://codereview.chromium.org/2549773002/diff/480001/src/code-stub-assemble...
src/code-stub-assembler.cc:3586: Bind(&deref);
STATIC_ASSERT(ConsString::kFirstOffset == ThinString::kActualOffset);

[Jakob Kummerow, 2017-01-19 10:20:16]

Thanks, nit addressed.

https://codereview.chromium.org/2549773002/diff/480001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2549773002/diff/480001/src/code-stub-assemble...
src/code-stub-assembler.cc:3583: GotoIf(WordEqual(rhs, EmptyStringConstant()),
&deref);
On 2017/01/18 14:55:20, Igor Sheludko wrote:
> Maybe we should stop using ConsStrings this way once ThinString sticks in the
> tree.

Yes, maybe. 
Pro: code checking for indirect strings (like here) would be simpler. 
Con: currently we have the invariant that a ThinString always refers to an
internalized string, whereas a flat ConsString can refer to a non-internalized
sequential string. If we replaced flat ConsStrings with ThinStrings, then we'd
either have to internalize more strings, or add internalization checks in the IC
handlers after deref'ing a ThinString.

https://codereview.chromium.org/2549773002/diff/480001/src/code-stub-assemble...
src/code-stub-assembler.cc:3586: Bind(&deref);
On 2017/01/18 14:55:20, Igor Sheludko wrote:
> STATIC_ASSERT(ConsString::kFirstOffset == ThinString::kActualOffset);

Done.

[Jakob Kummerow, 2017-01-19 10:20:26]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2017-01-19 10:20:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, hpayer@chromium.org, yangguo@chromium.org,
ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2549773002/#ps500001
(title: "fix nit")

[commit-bot: I haz the power, 2017-01-19 10:20:31]

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-19 10:22:20]

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

[commit-bot: I haz the power, 2017-01-19 10:22:22]

Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/19509)

[Jakob Kummerow, 2017-01-19 12:49:52]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2017-01-19 12:49:52]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org,
bmeurer@chromium.org, hpayer@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2549773002/#ps520001
(title: "rebased")

[commit-bot: I haz the power, 2017-01-19 12:49:57]

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-19 13:28:01]

CQ is committing da patch.

Bot data: {"patchset_id": 520001, "attempt_start_ts": 1484830192024680,
"parent_rev": "34b337fae6bc7c354ec76ada6e10e21e492492e4", "commit_rev":
"3e915e12a146fb346765ab5027c5e9ebad0600f9"}

[commit-bot: I haz the power, 2017-01-19 13:28:07]

Description was changed from

==========
Internalize strings in-place (reland^3)

using newly introduced ThinStrings, which store a pointer to the actual,
internalized string they represent.

BUG=v8:4520

(Previously landed as #42168 / af51befe694f)
(Previously landed as #42193 / 4c699e349a49)
(Previously landed as #42235 / ec45e6ed2e11)
==========

to

==========
Internalize strings in-place (reland^3)

using newly introduced ThinStrings, which store a pointer to the actual,
internalized string they represent.

BUG=v8:4520

(Previously landed as #42168 / af51befe694f)
(Previously landed as #42193 / 4c699e349a49)
(Previously landed as #42235 / ec45e6ed2e11)

Review-Url: https://codereview.chromium.org/2549773002
Cr-Commit-Position: refs/heads/master@{#42503}
Committed:
https://chromium.googlesource.com/v8/v8/+/3e915e12a146fb346765ab5027c5e9ebad0...
==========

[commit-bot: I haz the power, 2017-01-19 13:28:09]

Committed patchset #27 (id:520001) as
https://chromium.googlesource.com/v8/v8/+/3e915e12a146fb346765ab5027c5e9ebad0...