Non-template base classes for TracedArray and TracedDictionary.

Non-template base classes for TracedArray and TracedDictionary.

These base classes allow modular programming where the owner context is not known.

R=yurys@chromium.org
BUG=

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

[zerny-chromium, 2014-07-09 14:16:08]

+pfeldman: since you did the owners review in
https://codereview.chromium.org/356013002

[yurys, 2014-07-09 15:22:03]

This way we will lose type-safety. I don't thing this builders should be used by
several modules. I would expect that all data for trace event arguments are
always available at the point where the event is recorded. What is the scenario
when owner type is not known?

[zerny-chromium, 2014-07-10 06:50:48]

On 2014/07/09 15:22:03, yurys wrote:
> This way we will lose type-safety. I don't thing this builders should be used
by
> several modules. I would expect that all data for trace event arguments are
> always available at the point where the event is recorded. What is the
scenario
> when owner type is not known?

By modular, I mean that the code constructing a TracedValue can be described by
several functions.

We are using the tracing infrastructure to do object snapshots of the oilpan
heap to get some extra profiling information. The code is naturally split up as
methods on the objects that are being taken a snapshot of. The current code
dictates that the static recursion depth of the program be encoded in the static
type, thus a method that takes a TracedValue in which to put its snapshot data
must know the entire TracedValue context.

It is not always possible to know the context since the same method might output
the same structure in two different contexts. Thus it would need to be templated
too. It is also not always possible to template such a method since it might be
virtual. Even if it is not virtual, templating the method causes the size of the
output program would be a function over the size of the lexical scoping depth
(duplicated for each different depth which needs to be statically known) for
code that need no specialization.

For example, I don't see any way to code the following case with the current
builder:

  virtual void Foo::snapshot(TracedArray<?>& json)
  {
    // output data for Foo
  }

  // Bar has a Bar m_next and Foo m_foo
  virtual void Bar::snapshot(TracedDictionary<?> json)
  {
     if (this->baseCase()) {
       TracedArray<?>& array = json.beginArray("MyFoo");
       m_foo->snapshot(array);
     } else {
       TracedDictionary<?>& dict = json.beginDictionay("NextBar");
       m_next->snapshot(dict);
     }
  }

I'm a big fan of type safety, but the interface is no less type safe after this
change (notice that the endArray/endDictionary are void methods). The code is
not type safe as it stands because of the implicit type changes done by
reinterpret_cast of |this|. For example, the following error is unsafe (and
gives an error in debug builds):

  TracedArray<TracedValue>& array = json.beginArray("MyArray");
  TracedDictionary<TracedArray<TracedValue> >& dict = array.beginDictionary();
  // Any use of |array| is type unsafe until dict.endDictionary() is called.

[pfeldman, 2014-07-10 07:17:48]

I wonder if trace events are a good fit for serializing the heap graphs. +nduca
on his thoughts on this. Whatever happens here needs to be coordinated with
base/. It might well be that we keep tracing focused on perf and keep heavy data
object serialization separate.

[yurys, 2014-07-10 08:22:19]

On 2014/07/10 06:50:48, zerny-chromium wrote:
> On 2014/07/09 15:22:03, yurys wrote:
> > This way we will lose type-safety. I don't thing this builders should be
used
> by
> > several modules. I would expect that all data for trace event arguments are
> > always available at the point where the event is recorded. What is the
> scenario
> > when owner type is not known?
> 
> By modular, I mean that the code constructing a TracedValue can be described
by
> several functions.
> 
> We are using the tracing infrastructure to do object snapshots of the oilpan
> heap to get some extra profiling information. The code is naturally split up
as
> methods on the objects that are being taken a snapshot of. The current code
> dictates that the static recursion depth of the program be encoded in the
static
> type, thus a method that takes a TracedValue in which to put its snapshot data
> must know the entire TracedValue context.
> 
> It is not always possible to know the context since the same method might
output
> the same structure in two different contexts. Thus it would need to be
templated
> too.
That was the idea. If there are several places that can share same routine for
constructing a part of tracing argument bug within different contexts, the
routine should be a template.

> It is also not always possible to template such a method since it might be
> virtual.
Yeah, template based approach obviously won't work if you want to pass a builder
with unknown context into a virtual method or if you don't know depth of the
resulting structure statically. The latter basically means that you need to
serialize a random object whose structure/recursive depth isn't known at compile
time. These classes were intended to be used for constructing tracing arguments
whose type is defined at compile time.

> Even if it is not virtual, templating the method causes the size of the
> output program would be a function over the size of the lexical scoping depth
> (duplicated for each different depth which needs to be statically known) for
> code that need no specialization.
> 
That's the price we pay for type-safety in this case. Hopefully the compiler
will be smart enough to optimize it.

> For example, I don't see any way to code the following case with the current
> builder:
> 
>   virtual void Foo::snapshot(TracedArray<?>& json)
>   {
>     // output data for Foo
>   }
> 
>   // Bar has a Bar m_next and Foo m_foo
>   virtual void Bar::snapshot(TracedDictionary<?> json)
>   {
>      if (this->baseCase()) {
>        TracedArray<?>& array = json.beginArray("MyFoo");
>        m_foo->snapshot(array);
>      } else {
>        TracedDictionary<?>& dict = json.beginDictionay("NextBar");
>        m_next->snapshot(dict);
>      }
>   }
> 
> I'm a big fan of type safety, but the interface is no less type safe after
this
> change (notice that the endArray/endDictionary are void methods). The code is
> not type safe as it stands because of the implicit type changes done by
> reinterpret_cast of |this|. For example, the following error is unsafe (and
> gives an error in debug builds):
> 
>   TracedArray<TracedValue>& array = json.beginArray("MyArray");
>   TracedDictionary<TracedArray<TracedValue> >& dict = array.beginDictionary();
>   // Any use of |array| is type unsafe until dict.endDictionary() is called.
Yeah, I don't see how we could easily prohibit modification of outer container
when there is open nested one using C++ type system. The debug asserts should
catch this.

Knowing the context allows to catch the cases when you close nested container
and tries to use a wrong type of its owner, i.e. the following code will result
in a compile-time error in the templated version but will compile if it uses
types from this change:
json.beginArray("MyArray")
    .beginDictionary()
    .endDictionary()
    .setInteger("foo", 1)


We could switch to a non-template version that provides less type-safety (one of
the extreme options would be to expose all methods right on TracedValue and rely
just on debug asserts to validate that produced output is a valid JSON). I'm not
sure if it makes sense to keep both template version and the one you are adding
in this change. I share Pavel's doubts regarding serialization of the heap graph
into trace events and would like to hear other's opinion.

@nduca, @dsinclair: what do you think?

[zerny-chromium, 2014-07-10 12:48:31]

> > It is also not always possible to template such a method since it might be
> > virtual.
>
> Yeah, template based approach obviously won't work if you want to pass a
builder
> with unknown context into a virtual method or if you don't know depth of the
> resulting structure statically. The latter basically means that you need to
> serialize a random object whose structure/recursive depth isn't known at
compile
> time. These classes were intended to be used for constructing tracing
arguments
> whose type is defined at compile time.

The example I provided basically amounts to a list type (ie, the recursive type
\X.1*X), so the type is well defined in advance, only its size is not known.

> > Even if it is not virtual, templating the method causes the size of the
> > output program would be a function over the size of the lexical scoping
depth
> > (duplicated for each different depth which needs to be statically known) for
> > code that need no specialization.
>
> That's the price we pay for type-safety in this case. Hopefully the compiler
> will be smart enough to optimize it.

Again, the only "type safety" difference after this change is that the recursion
depth is not statically known, thus a runtime check must be in place when
popping elements of the stack.

> Yeah, I don't see how we could easily prohibit modification of outer container
> when there is open nested one using C++ type system. The debug asserts should
> catch this.

Yes, and an assert can catch incorrect nesting too.
 
> Knowing the context allows to catch the cases when you close nested container
> and tries to use a wrong type of its owner, i.e. the following code will
result
> in a compile-time error in the templated version but will compile if it uses
> types from this change:
> json.beginArray("MyArray")
>     .beginDictionary()
>     .endDictionary()
>     .setInteger("foo", 1)

This won't work because endDictionary() returns void so you can't do .setInteger
on an incorrectly typed value.

> We could switch to a non-template version that provides less type-safety (one
of
> the extreme options would be to expose all methods right on TracedValue and
rely
> just on debug asserts to validate that produced output is a valid JSON).

That is not the case with this CL. It still type distinguishes Array and
Dictionary so we have the same degree of "type safety" as before. Only the
nesting check is no longer statically checked.

> I'm not sure if it makes sense to keep both template version and the one you
are adding
> in this change. I share Pavel's doubts regarding serialization of the heap
graph
> into trace events and would like to hear other's opinion.

I'm not in position to make that judgment. We have started putting more info
into the tracing infrastructure (some behind a compile flag) so to allow using
the trace-viewer to visualize heap consumption/growth/object-lifetime and do so
while also having the additional information about what is going on the the rest
of the system timeline-wise. That seems like a reasonable use case to me. Also,
this way we can use telemetry to collect trace files for existing page sets too.

> @nduca, @dsinclair: what do you think?

[zerny-chromium, 2014-07-10 12:50:50]

On 2014/07/10 07:17:48, pfeldman wrote:
> I wonder if trace events are a good fit for serializing the heap graphs.
+nduca
> on his thoughts on this. Whatever happens here needs to be coordinated with
> base/. It might well be that we keep tracing focused on perf and keep heavy
data
> object serialization separate.

This is for profiling purposes (and the snapshot infrastructure is used for
similar stuff for tcmalloc AFAIK).

[pfeldman, 2014-07-10 14:05:05]

> This is for profiling purposes (and the snapshot infrastructure is used for
> similar stuff for tcmalloc AFAIK).

Could you elaborate on the details? Design docs / plans / anything? We were
planning on adding an Oilpan heap profiler into DevTools for internal use + for
the native memory profiling we could expose to the users. I wonder if there is
something we should be reusing.

[zerny-chromium, 2014-07-10 14:12:47]

On 2014/07/10 14:05:05, pfeldman wrote:
> > This is for profiling purposes (and the snapshot infrastructure is used for
> > similar stuff for tcmalloc AFAIK).
> 
> Could you elaborate on the details? Design docs / plans / anything? We were
> planning on adding an Oilpan heap profiler into DevTools for internal use +
for
> the native memory profiling we could expose to the users. I wonder if there is
> something we should be reusing.

Sure. We don't have a design doc since right now we just need it for internal
dev while pinning down performance issues. Thus we are putting it behind a flag.
Long term I'm sure there are parts of it we can pull out of the flag for general
usage.

I'll upload a CL with tentative stuff shortly. That will provide more context.

[alph, 2014-07-10 14:45:23]

https://codereview.chromium.org/381653002/diff/1/Source/platform/TracedValue.h
File Source/platform/TracedValue.h (right):

https://codereview.chromium.org/381653002/diff/1/Source/platform/TracedValue....
Source/platform/TracedValue.h:143: TracedDictionaryBase& beginDictionary()
You could return TracedDictionary<TracedArrayBase> at his point instead.

This will allow you to temporary forget the current context and pass the
contextless dictionary to a function that does not need to know about the
context. The function in turn may still continue adding contexts if it needs to.

wdyt?

Note that you need to adjust the nestingLevel checks though, as these become
dynamic.

[zerny-chromium, 2014-07-10 15:04:06]

On 2014/07/10 14:45:23, alph wrote:
> https://codereview.chromium.org/381653002/diff/1/Source/platform/TracedValue.h
> File Source/platform/TracedValue.h (right):
> 
>
https://codereview.chromium.org/381653002/diff/1/Source/platform/TracedValue....
> Source/platform/TracedValue.h:143: TracedDictionaryBase& beginDictionary()
> You could return TracedDictionary<TracedArrayBase> at his point instead.
> 
> This will allow you to temporary forget the current context and pass the
> contextless dictionary to a function that does not need to know about the
> context. The function in turn may still continue adding contexts if it needs
to.
> 
> wdyt?

That sounds like a good idea.

> Note that you need to adjust the nestingLevel checks though, as these become
> dynamic.

Yes. The nesting check is needed and is currently in
TracedValueBase::endCurrent{Dictionary,Array}, which asserts that the stack is
non-empty and that the top element has the right "type tag".

Regarding the heap profiling code, I've uploaded a CL at:
https://codereview.chromium.org/383743002/
Its still unfinished after refactoring from RefPtr<JSONValue> to the TracedValue
so there are a few issues still. However, it shows what I was expecting to land.
Mostly just for other oilpan team members to get profiling data while we are
increasing performance of the system.

We'd be happy for alternatives, since we can't currently snapshot all of the
page header information which is just too much data for a trace-event snapshot
(especially since it need to be a json object).

[yurys, 2014-07-10 15:14:07]

https://codereview.chromium.org/381653002/diff/1/Source/platform/TracedValue.h
File Source/platform/TracedValue.h (right):

https://codereview.chromium.org/381653002/diff/1/Source/platform/TracedValue....
Source/platform/TracedValue.h:143: TracedDictionaryBase& beginDictionary()
On 2014/07/10 14:45:22, alph wrote:
> You could return TracedDictionary<TracedArrayBase> at his point instead.
> 
> This will allow you to temporary forget the current context and pass the
> contextless dictionary to a function that does not need to know about the
> context. The function in turn may still continue adding contexts if it needs
to.
> 
> wdyt?
> 
Sounds good to me too. It would help to resolve the issue with modular
programming.

[dsinclair, 2014-07-10 15:32:04]

On 2014/07/10 07:17:48, pfeldman wrote:
> I wonder if trace events are a good fit for serializing the heap graphs.
+nduca
> on his thoughts on this. Whatever happens here needs to be coordinated with
> base/. It might well be that we keep tracing focused on perf and keep heavy
data
> object serialization separate.


Tracing already has some pretty heavy objects in it (CC layer trees, SkPictures)
and we're hoping to add more in the future. There is a number of large
structures we want to expose in trace-viewer. The use of a Convertable and
disabled-by-default should, hopefully, keep these having minimal impact when not
enabled.

[dsinclair, 2014-07-10 15:36:08]

On 2014/07/10 12:48:31, zerny-chromium wrote:
 > I'm not sure if it makes sense to keep both template version and the one you
> are adding
> > in this change. I share Pavel's doubts regarding serialization of the heap
> graph
> > into trace events and would like to hear other's opinion.
> 
> I'm not in position to make that judgment. We have started putting more info
> into the tracing infrastructure (some behind a compile flag) so to allow using
> the trace-viewer to visualize heap consumption/growth/object-lifetime and do
so
> while also having the additional information about what is going on the the
rest
> of the system timeline-wise. That seems like a reasonable use case to me.
Also,
> this way we can use telemetry to collect trace files for existing page sets
too.


This sounds like it would be awesome in tracing. In theory you shouldn't need
the compile time flag. You can use disabled-by-default categories (and if that
is still too slow, we can do a bit better perf wise, similar to what we do with
the CC layer tree)

[zerny-chromium, 2014-07-11 07:20:30]

Latest patch set now returns templated types for the begin{Array,Dictionary}
methods. This change comes at the cost of not having the static nesting depth
anymore since it is not possible to statically compute it from an unknown
context. However, tag correctness and stack empty/non-empty checks are still
valid.

I've also added a typedef for the type of |this| for readability.

[yurys, 2014-07-11 08:00:21]

LGTM

On 2014/07/11 07:20:30, zerny-chromium wrote:
> Latest patch set now returns templated types for the begin{Array,Dictionary}
> methods. This change comes at the cost of not having the static nesting depth
> anymore since it is not possible to statically compute it from an unknown
> context. However, tag correctness and stack empty/non-empty checks are still
> valid.
> 
> I've also added a typedef for the type of |this| for readability.
Yeah, this way it is more readable.

[pfeldman, 2014-07-11 12:10:43]

lgtm

[zerny-chromium, 2014-07-11 12:19:53]

Thanks for the feedback guys! I'll be reaching out to you when back from
vacation (in 14 days) about further work on profiling the blink gc.

[zerny-chromium, 2014-07-11 12:19:57]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-07-11 12:21:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/381653002/20001

[yurys, 2014-07-11 12:21:50]

https://codereview.chromium.org/381653002/diff/20001/Source/platform/TracedVa...
File Source/platform/TracedValue.h (right):

https://codereview.chromium.org/381653002/diff/20001/Source/platform/TracedVa...
Source/platform/TracedValue.h:54: void endDictionary()
It seems that this method can be removed since creator of TracedDictionaryBase
should always know the context and could use template version of endDictionary.

[zerny-chromium, 2014-07-11 12:36:59]

The CQ bit was unchecked by zerny@chromium.org

[zerny-chromium, 2014-07-11 12:37:18]

https://codereview.chromium.org/381653002/diff/20001/Source/platform/TracedVa...
File Source/platform/TracedValue.h (right):

https://codereview.chromium.org/381653002/diff/20001/Source/platform/TracedVa...
Source/platform/TracedValue.h:54: void endDictionary()
On 2014/07/11 12:21:50, yurys wrote:
> It seems that this method can be removed since creator of TracedDictionaryBase
> should always know the context and could use template version of
endDictionary.

Good point, I'll get that change it right away.

[zerny-chromium, 2014-07-11 12:57:17]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-07-11 12:57:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/381653002/40001

[zerny-chromium, 2014-07-11 13:23:29]

On 2014/07/11 12:37:18, zerny-chromium wrote:
>
https://codereview.chromium.org/381653002/diff/20001/Source/platform/TracedVa...
> File Source/platform/TracedValue.h (right):
> 
>
https://codereview.chromium.org/381653002/diff/20001/Source/platform/TracedVa...
> Source/platform/TracedValue.h:54: void endDictionary()
> On 2014/07/11 12:21:50, yurys wrote:
> > It seems that this method can be removed since creator of
TracedDictionaryBase
> > should always know the context and could use template version of
> endDictionary.
> 
> Good point, I'll get that change it right away.

While it works to not have the endX methods on the bases it does not make for
nice looking code:
https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/He...

We can omit them now, but it might make the code nicer to add them back.
Alternatively we should add a support for a scoped value ala "TracedArrayScope
array(parent);" that implicitly begins and ends. That would make a lot of the
heap code nicer at would avoid the issue of having an incorrectly typed variable
hanging around after the call to endX.

[commit-bot: I haz the power, 2014-07-11 14:02:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/15069)

[commit-bot: I haz the power, 2014-07-11 14:36:15]

Change committed as 177945