Out-of-line WTF::String's destructor.

Out-of-line WTF::String's destructor.

This saves ~504KB in the official Linux x64 binary.

BUG=none

Committed: https://crrev.com/2ce3d703c50150ebe5cd6c3f1e2bf4a19f590766
Cr-Commit-Position: refs/heads/master@{#433737}

[dcheng, 2016-11-20 04:54:29]

dcheng@chromium.org changed reviewers:
+ esprehn@chromium.org

[dcheng, 2016-11-20 05:07:44]

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

[commit-bot: I haz the power, 2016-11-20 05:07:48]

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-11-20 07:08:23]

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

[commit-bot: I haz the power, 2016-11-20 07:08:24]

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

[dcheng, 2016-11-20 08:22:52]

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

[commit-bot: I haz the power, 2016-11-20 08:22:58]

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-11-20 09:56:49]

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

[commit-bot: I haz the power, 2016-11-20 09:56:50]

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

[dcheng, 2016-11-20 10:00:44]

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

[commit-bot: I haz the power, 2016-11-20 10:00: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, 2016-11-20 11:26:00]

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

[commit-bot: I haz the power, 2016-11-20 11:26:00]

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

[esprehn, 2016-11-20 20:44:27]

Can we do StringImpl instead? I'd rather not mess with String, I assume the
bloat is coming from the stuff inside ref and deref on the Impl?

[dcheng, 2016-11-20 21:46:52]

On 2016/11/20 20:44:27, esprehn wrote:
> Can we do StringImpl instead? I'd rather not mess with String, I assume the
> bloat is coming from the stuff inside ref and deref on the Impl?

Unfortunately, it's largely from String's destructor itself:

     Count      Bytes   Name
  --------  ---------   ---------------------------------
     34500     972306   WTF::RefPtr<WTF::StringImpl>::~RefPtr()
     31576     898555   WTF::String::~String()
     34630     843076   void
WTF::derefIfNotNull<WTF::StringImpl>(WTF::StringImpl*)

(Note there's some double-counting here: a large portion of the
~RefPtr<StringImpl> bytes should be attributed to ~String, but I haven't fixed
that part of the tool yet.)

StringImpl's destructor is already defined out-of-line: despite the fact that
it's annotated with inline, the compiler doesn't appear to be inlining it
already.

[Mikhail, 2016-11-21 06:51:01]

mikhail.pozdnyakov@intel.com changed reviewers:
+ mikhail.pozdnyakov@intel.com

[Mikhail, 2016-11-21 06:51:02]

https://codereview.chromium.org/2516123002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/text/WTFString.h (right):

https://codereview.chromium.org/2516123002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/WTFString.h:102: ~String();
This will remove implicitly-declared move constructor, so this change should be
accompanied with introducing of explicit move semantics to WTF::String.

[dcheng, 2016-11-21 18:10:40]

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

[commit-bot: I haz the power, 2016-11-21 18:11:04]

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-11-21 20:03:58]

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

[commit-bot: I haz the power, 2016-11-21 20:03:58]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-11-22 01:30:08]

https://codereview.chromium.org/2516123002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/text/WTFString.h (right):

https://codereview.chromium.org/2516123002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/WTFString.h:102: ~String();
On 2016/11/21 06:51:02, Mikhail wrote:
> This will remove implicitly-declared move constructor, so this change should
be
> accompanied with introducing of explicit move semantics to WTF::String.

Ah, good catch. I was wondering about the ASAN failures. Done.

[esprehn, 2016-11-22 01:41:11]

Do you want to change AtomicString too?

I do worry this is going to be bad for performance, especially inside some
loops, but lets give it a shot.

lgtm

[dcheng, 2016-11-22 01:47:25]

On 2016/11/22 01:41:11, esprehn wrote:
> Do you want to change AtomicString too?

Let's see how this goes first =)

> 
> I do worry this is going to be bad for performance, especially inside some
> loops, but lets give it a shot.

I'm a bit concerned as well, so I'd like to see how this goes before extending
it further: there are a couple other String() things that could be inlined, and
it might be nice to separate them out in case some of them do affect performance
negatively (or positively).

> 
> lgtm

[dcheng, 2016-11-22 01:47:32]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-11-22 01:48:11]

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-11-22 01:55:11]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479779252371060,
"parent_rev": "f2d77f028bad51f29d2d011b298190edac5b2e31", "commit_rev":
"ea3362bf961fd44824bdf9b8acac8a8e5c553398"}

[commit-bot: I haz the power, 2016-11-22 01:55:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-22 02:08:09]

Description was changed from

==========
Out-of-line WTF::String's destructor.

This saves ~504KB in the official Linux x64 binary.

BUG=none
==========

to

==========
Out-of-line WTF::String's destructor.

This saves ~504KB in the official Linux x64 binary.

BUG=none

Committed: https://crrev.com/2ce3d703c50150ebe5cd6c3f1e2bf4a19f590766
Cr-Commit-Position: refs/heads/master@{#433737}
==========

[commit-bot: I haz the power, 2016-11-22 02:08:10]

Patchset 2 (id:??) landed as
https://crrev.com/2ce3d703c50150ebe5cd6c3f1e2bf4a19f590766
Cr-Commit-Position: refs/heads/master@{#433737}

[dcheng, 2016-11-23 19:07:51]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2522243003/ by dcheng@chromium.org.

The reason for reverting is: 10.7% regression in blink_perf.dom =(.