[es2015] Precompute the descriptive string for symbols.

[es2015] Precompute the descriptive string for symbols.

Previously the String constructor and the Symbol.prototype.toString
methods had to compute the descriptive string for a Symbol on the fly,
which can produce a lot of garbage when this happens a lot, i.e. when
the String representation of a Symbol is used often. Now instead of
doing this on-demand we can just do it upfront when creating the Symbol.

That way we also ensure that we won't throw an exception when accessing
the descriptive string of a Symbol, due to potential String length
overflow, but have the exception during Symbol creation upfront, which
is a lot less surprising behavior.

BUG=v8:6278, v8:6344, v8:6350
TBR=mlippautz@chromium.org
R=ishell@chromium.org

Review-Url: https://codereview.chromium.org/2900703002
Cr-Commit-Position: refs/heads/master@{#45479}
Committed: https://chromium.googlesource.com/v8/v8/+/e87573822e1c0c041c03f2b60599b0ab9256422f

[Benedikt Meurer, 2017-05-22 08:57:32]

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

[commit-bot: I haz the power, 2017-05-22 08:57:34]

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-05-22 09:04:05]

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

[commit-bot: I haz the power, 2017-05-22 09:04:06]

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

[Benedikt Meurer, 2017-05-22 09:12:33]

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

[commit-bot: I haz the power, 2017-05-22 09:12:37]

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-05-22 09:43:06]

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

[commit-bot: I haz the power, 2017-05-22 09:43:07]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2017-05-23 07:08:24]

lgtm with nits:

https://codereview.chromium.org/2900703002/diff/20001/src/builtins/mips64/bui...
File src/builtins/mips64/builtins-mips64.cc (right):

https://codereview.chromium.org/2900703002/diff/20001/src/builtins/mips64/bui...
src/builtins/mips64/builtins-mips64.cc:303: __ ld(v0, FieldMemOperand(a0,
Symbol::kDescriptiveStringOffset));
Usually we use Ld() instead of ld() although the latter is also correct.

https://codereview.chromium.org/2900703002/diff/20001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2900703002/diff/20001/src/factory.cc#newcode931
src/factory.cc:931:
symbol->set_descriptive_string(String::cast(*descriptive_string));
String::cast is not necessary here.

https://codereview.chromium.org/2900703002/diff/20001/src/factory.cc#newcode936
src/factory.cc:936: Handle<Symbol> Factory::NewPrivateSymbol() {
Is this method still used?

https://codereview.chromium.org/2900703002/diff/20001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/2900703002/diff/20001/src/factory.h#newcode269
src/factory.h:269: Handle<Symbol> NewSymbol();
Please move this helper method to the private section of a class or inline it to
avoid confusion.

https://codereview.chromium.org/2900703002/diff/20001/src/runtime/runtime-sym...
File src/runtime/runtime-symbol.cc (right):

https://codereview.chromium.org/2900703002/diff/20001/src/runtime/runtime-sym...
src/runtime/runtime-symbol.cc:35:
RUNTIME_FUNCTION(Runtime_SymbolDescriptiveString) {
Why is this function still necessary?

[Benedikt Meurer, 2017-05-23 08:37:40]

https://codereview.chromium.org/2900703002/diff/20001/src/builtins/mips64/bui...
File src/builtins/mips64/builtins-mips64.cc (right):

https://codereview.chromium.org/2900703002/diff/20001/src/builtins/mips64/bui...
src/builtins/mips64/builtins-mips64.cc:303: __ ld(v0, FieldMemOperand(a0,
Symbol::kDescriptiveStringOffset));
On 2017/05/23 07:08:23, Igor Sheludko wrote:
> Usually we use Ld() instead of ld() although the latter is also correct.

Done.

https://codereview.chromium.org/2900703002/diff/20001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2900703002/diff/20001/src/factory.cc#newcode931
src/factory.cc:931:
symbol->set_descriptive_string(String::cast(*descriptive_string));
On 2017/05/23 07:08:23, Igor Sheludko wrote:
> String::cast is not necessary here.

Done.

https://codereview.chromium.org/2900703002/diff/20001/src/factory.cc#newcode936
src/factory.cc:936: Handle<Symbol> Factory::NewPrivateSymbol() {
On 2017/05/23 07:08:23, Igor Sheludko wrote:
> Is this method still used?

Yes.

https://codereview.chromium.org/2900703002/diff/20001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/2900703002/diff/20001/src/factory.h#newcode269
src/factory.h:269: Handle<Symbol> NewSymbol();
On 2017/05/23 07:08:23, Igor Sheludko wrote:
> Please move this helper method to the private section of a class or inline it
to
> avoid confusion.

Done.

https://codereview.chromium.org/2900703002/diff/20001/src/runtime/runtime-sym...
File src/runtime/runtime-symbol.cc (right):

https://codereview.chromium.org/2900703002/diff/20001/src/runtime/runtime-sym...
src/runtime/runtime-symbol.cc:35:
RUNTIME_FUNCTION(Runtime_SymbolDescriptiveString) {
On 2017/05/23 07:08:23, Igor Sheludko wrote:
> Why is this function still necessary?

Yes for the debugger.

[Benedikt Meurer, 2017-05-23 08:40:31]

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

[commit-bot: I haz the power, 2017-05-23 08:40:37]

Dry run: CQ is trying da patch.

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

[Benedikt Meurer, 2017-05-23 08:40:41]

The CQ bit was unchecked by bmeurer@chromium.org

[Benedikt Meurer, 2017-05-23 08:40:43]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2017-05-23 08:40:44]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2900703002/#ps40001
(title: "Address feedback.")

[commit-bot: I haz the power, 2017-05-23 08:40:47]

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-05-23 09:49:09]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495528843639520,
"parent_rev": "70a43f4d3dccf5b4079459f14423d0d9f1c13d80", "commit_rev":
"e87573822e1c0c041c03f2b60599b0ab9256422f"}

[commit-bot: I haz the power, 2017-05-23 09:49:17]

Description was changed from

==========
[es2015] Precompute the descriptive string for symbols.

Previously the String constructor and the Symbol.prototype.toString
methods had to compute the descriptive string for a Symbol on the fly,
which can produce a lot of garbage when this happens a lot, i.e. when
the String representation of a Symbol is used often. Now instead of
doing this on-demand we can just do it upfront when creating the Symbol.

That way we also ensure that we won't throw an exception when accessing
the descriptive string of a Symbol, due to potential String length
overflow, but have the exception during Symbol creation upfront, which
is a lot less surprising behavior.

BUG=v8:6278,v8:6344,v8:6350
TBR=mlippautz@chromium.org
R=ishell@chromium.org
==========

to

==========
[es2015] Precompute the descriptive string for symbols.

Previously the String constructor and the Symbol.prototype.toString
methods had to compute the descriptive string for a Symbol on the fly,
which can produce a lot of garbage when this happens a lot, i.e. when
the String representation of a Symbol is used often. Now instead of
doing this on-demand we can just do it upfront when creating the Symbol.

That way we also ensure that we won't throw an exception when accessing
the descriptive string of a Symbol, due to potential String length
overflow, but have the exception during Symbol creation upfront, which
is a lot less surprising behavior.

BUG=v8:6278,v8:6344,v8:6350
TBR=mlippautz@chromium.org
R=ishell@chromium.org

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

[commit-bot: I haz the power, 2017-05-23 09:49:18]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/e87573822e1c0c041c03f2b60599b0ab925...

[Michael Lippautz, 2017-05-23 11:16:12]

On 2017/05/23 09:49:18, commit-bot: I haz the power wrote:
> Committed patchset #3 (id:40001) as
>
https://chromium.googlesource.com/v8/v8/+/e87573822e1c0c041c03f2b60599b0ab925...

lgtm

[Michael Achenbach, 2017-05-23 11:57:54]

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

The reason for reverting is: Speculative revert for:
https://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%20debug/builds....

[Michael Achenbach, 2017-05-23 12:58:06]

Turned green after revert:
https://build.chromium.org/p/client.v8/builders/V8%20Win32%20-%20debug/builds...