Remove Blink's ConvertableToTraceFormat

Remove Blink's ConvertableToTraceFormat

In short, to estimate memory overhead correctly.

Rationale:
- With the current design, we need to (1) add sizeof(TracedValue) and
  sizeof(ConvertableToTraceFormatWrapper), and (2) allocate them on malloc
  (not on PartitionAlloc/Oilpan) to estimate memory overhead correctly
- TracedValue is the only instance of ConvertableToTraceFormat and TracedValue
  is basically a wrapper of base::trace_event::TracedValue
- The purpose of ConvertableToTraceFormat is to avoid unneeded conversion but
  most of Blink objects (e.g. Strings, Vectors, Nodes) are allocated on
  PartitionAlloc/Oilpan so it's almost impossible to avoid conversion in Blink.
  ConvertableToTraceFormat isn't as useful as in base/
- If we only use TracedValue for structured values (this is true at this point)
  we can just pass base::trace_event::TracedValue directly to base/

Bonus points:
- We can remove ConvertableToTraceFormatWrapper and string copies.
- We can remove public methods which expose base/ types to core/modules.

BUG=570961
Committed: https://crrev.com/a98a9dd2baa5ef054efb48336ee36b27b48b2c94
Cr-Commit-Position: refs/heads/master@{#372298}

[bashi, 2016-01-27 03:15:04]

Patchset #1 (id:1) has been deleted

[bashi, 2016-01-27 04:29:23]

Patchset #1 (id:20001) has been deleted

[bashi, 2016-01-27 04:42:13]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, primiano@chromium.org

[bashi, 2016-01-27 04:42:14]

I think removing ConvertableToTraceFormat from blink is the way to go as I wrote
in the description. Another options would be:
- Add overhead estimation mechanism to PartitionAlloc (how?)
- Allocate blink's ConvertableToTraceFormat by malloc. In other words, stop
inheriting from RefCounted.

WDYT?

[Primiano Tucci (use gerrit), 2016-01-28 10:31:44]

Let me see if my understanding is correct: today in blink we have
ConvertableToTraceFormat (with all the wrapper boilerplat) but the only use of
that Convertable is blink::TracedValue ?
If this is the case, this LGTM. I agree is just simpler to have the TracedValue
being malloc-ed.
Also this aligns to my plan for tracing to make TracedValue a first class
citizen and incentivating people to use directly that, and reserve the
Convertable for very limited cases.

FYI, one thing to keep in mind (which I think is orthogonal with this CL) is
that soon we'll make base::ConvertableToTraceFormat NON-refcounted (and pass by
scoped_ptr) as it turns out that creates races (see crbug.com/559117). Didn't
have the time to figure out what that entails concretely in the codebase though.
I think this CL is good regardless.
Many thanks for removing all these awkward layers

[hiroshige, 2016-01-28 12:05:00]

hiroshige@chromium.org changed reviewers:
+ hiroshige@chromium.org

[hiroshige, 2016-01-28 12:05:01]

It's great to remove layers around ConvertableToTraceFormat!

https://codereview.chromium.org/1620673002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/EventTracer.cpp (right):

https://codereview.chromium.org/1620673002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/EventTracer.cpp:82: convertables[0] =
convertableValue1->m_tracedValue;
Probably we have to move the scoped_ptr from |convertableValue1->m_tracedValue|
to |convertables[0]|, so that |convertableValue1->m_tracedValue| becomes null
after this line for thread safety,
as ConvertableToTraceFormatWrapper() did similar things.
(See comments in L49-50 in ConvertableToTraceFormatWrapper)

https://codereview.chromium.org/1620673002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/EventTracer.cpp:84: convertables[1] =
convertableValue2->m_tracedValue;
ditto.

https://codereview.chromium.org/1620673002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/TracedValue.h (right):

https://codereview.chromium.org/1620673002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/TracedValue.h:22: class PLATFORM_EXPORT
TracedValue : public RefCounted<TracedValue> {
TracedValue should be final (or ~TracedValue() should be virtual).
Previously, TraceEvent::ConvertableToTraceFormat's destructor was virtual.

[bashi, 2016-01-29 02:25:12]

On 2016/01/28 10:31:44, Primiano Tucci wrote:
> Let me see if my understanding is correct: today in blink we have
> ConvertableToTraceFormat (with all the wrapper boilerplat) but the only use of
> that Convertable is blink::TracedValue ?

Yes. blink::TracedValue is the only subclass of ConvertableToTraceFormat.

> If this is the case, this LGTM. I agree is just simpler to have the
TracedValue
> being malloc-ed.
> Also this aligns to my plan for tracing to make TracedValue a first class
> citizen and incentivating people to use directly that, and reserve the
> Convertable for very limited cases.
> 
> FYI, one thing to keep in mind (which I think is orthogonal with this CL) is
> that soon we'll make base::ConvertableToTraceFormat NON-refcounted (and pass
by
> scoped_ptr) as it turns out that creates races (see crbug.com/559117). Didn't
> have the time to figure out what that entails concretely in the codebase
though.
> I think this CL is good regardless.

I think blink's TracedValue could be non-refcounted as well. Actually I was
thinking to make TracedValue non-refcounted but didn't to keep this CL simple.

> Many thanks for removing all these awkward layers

[bashi, 2016-01-29 02:25:26]

https://codereview.chromium.org/1620673002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/EventTracer.cpp (right):

https://codereview.chromium.org/1620673002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/EventTracer.cpp:82: convertables[0] =
convertableValue1->m_tracedValue;
On 2016/01/28 12:05:01, hiroshige wrote:
> Probably we have to move the scoped_ptr from
|convertableValue1->m_tracedValue|
> to |convertables[0]|, so that |convertableValue1->m_tracedValue| becomes null
> after this line for thread safety,
> as ConvertableToTraceFormatWrapper() did similar things.
> (See comments in L49-50 in ConvertableToTraceFormatWrapper)

Good catch! Done.

https://codereview.chromium.org/1620673002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/TracedValue.h (right):

https://codereview.chromium.org/1620673002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/TracedValue.h:22: class PLATFORM_EXPORT
TracedValue : public RefCounted<TracedValue> {
On 2016/01/28 12:05:01, hiroshige wrote:
> TracedValue should be final (or ~TracedValue() should be virtual).
> Previously, TraceEvent::ConvertableToTraceFormat's destructor was virtual.

Done.

[kinuko, 2016-01-29 02:38:52]

Driving-by, just wanted to say that looks great, and if we could also make
TracedValue non-refcounted that sounds even nicer :D

[bashi, 2016-01-29 02:54:24]

On 2016/01/29 02:38:52, kinuko wrote:
> Driving-by, just wanted to say that looks great, and if we could also make
> TracedValue non-refcounted that sounds even nicer :D

I'll try to create a follow-up CL to make TracedValue non-refcounted then:) I'd
like to land this first if this looks reasonable.

[haraken, 2016-01-29 04:41:21]

LGTM

[bashi, 2016-01-29 05:27:06]

bashi@chromium.org changed reviewers:
+ yhirano@chromium.org

[bashi, 2016-01-29 05:27:06]

Hirano-san, could you review modules/websockets?

[yhirano, 2016-01-29 05:44:29]

lgtm

https://codereview.chromium.org/1620673002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/websockets/InspectorWebSocketEvents.cpp
(right):

https://codereview.chromium.org/1620673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/websockets/InspectorWebSocketEvents.cpp:8:
#include "platform/TracedValue.h"
nit: Not needed because the corresponding header includes it.

https://codereview.chromium.org/1620673002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/TraceEvent.h (right):

https://codereview.chromium.org/1620673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/TraceEvent.h:114: //                   
PassRefPtr<TracedValue> convertableValues[],
nit: "convertable" not needed?

[bashi, 2016-01-29 06:01:01]

https://codereview.chromium.org/1620673002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/websockets/InspectorWebSocketEvents.cpp
(right):

https://codereview.chromium.org/1620673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/websockets/InspectorWebSocketEvents.cpp:8:
#include "platform/TracedValue.h"
On 2016/01/29 05:44:29, yhirano wrote:
> nit: Not needed because the corresponding header includes it.

Done.

https://codereview.chromium.org/1620673002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/TraceEvent.h (right):

https://codereview.chromium.org/1620673002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/TraceEvent.h:114: //                   
PassRefPtr<TracedValue> convertableValues[],
On 2016/01/29 05:44:29, yhirano wrote:
> nit: "convertable" not needed?

Done. Also renamed moveFromIfConvertable().

[bashi, 2016-01-29 06:01:07]

The CQ bit was checked by bashi@chromium.org

[bashi, 2016-01-29 06:01:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, haraken@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1620673002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-01-29 06:02:34]

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

[commit-bot: I haz the power, 2016-01-29 07:15:24]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-01-29 07:16:13]

Description was changed from

==========
Remove Blink's ConvertableToTraceFormat

In short, to estimate memory overhead correctly.

Rationale:
- With the current design, we need to (1) add sizeof(TracedValue) and
  sizeof(ConvertableToTraceFormatWrapper), and (2) allocate them on malloc
  (not on PartitionAlloc/Oilpan) to estimate memory overhead correctly
- TracedValue is the only instance of ConvertableToTraceFormat and TracedValue
  is basically a wrapper of base::trace_event::TracedValue
- The purpose of ConvertableToTraceFormat is to avoid unneeded conversion but
  most of Blink objects (e.g. Strings, Vectors, Nodes) are allocated on
  PartitionAlloc/Oilpan so it's almost impossible to avoid conversion in Blink.
  ConvertableToTraceFormat isn't as useful as in base/
- If we only use TracedValue for structured values (this is true at this point)
  we can just pass base::trace_event::TracedValue directly to base/

Bonus points:
- We can remove ConvertableToTraceFormatWrapper and string copies.
- We can remove public methods which expose base/ types to core/modules.

BUG=570961
==========

to

==========
Remove Blink's ConvertableToTraceFormat

In short, to estimate memory overhead correctly.

Rationale:
- With the current design, we need to (1) add sizeof(TracedValue) and
  sizeof(ConvertableToTraceFormatWrapper), and (2) allocate them on malloc
  (not on PartitionAlloc/Oilpan) to estimate memory overhead correctly
- TracedValue is the only instance of ConvertableToTraceFormat and TracedValue
  is basically a wrapper of base::trace_event::TracedValue
- The purpose of ConvertableToTraceFormat is to avoid unneeded conversion but
  most of Blink objects (e.g. Strings, Vectors, Nodes) are allocated on
  PartitionAlloc/Oilpan so it's almost impossible to avoid conversion in Blink.
  ConvertableToTraceFormat isn't as useful as in base/
- If we only use TracedValue for structured values (this is true at this point)
  we can just pass base::trace_event::TracedValue directly to base/

Bonus points:
- We can remove ConvertableToTraceFormatWrapper and string copies.
- We can remove public methods which expose base/ types to core/modules.

BUG=570961

Committed: https://crrev.com/a98a9dd2baa5ef054efb48336ee36b27b48b2c94
Cr-Commit-Position: refs/heads/master@{#372298}
==========

[commit-bot: I haz the power, 2016-01-29 07:16:14]

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