Implement Composition.buildFromEvents()

tracing/tracing/value/diagnostics/composition.html

Index: tracing/tracing/value/diagnostics/composition.html
diff --git a/tracing/tracing/value/diagnostics/composition.html b/tracing/tracing/value/diagnostics/composition.html
index 5561efaf365e28fcc1b5886e72ed25eb00b929d4..7b087e212399ebb0fdd74a5ed00679b0dae0817e 100644
--- a/tracing/tracing/value/diagnostics/composition.html
+++ b/tracing/tracing/value/diagnostics/composition.html
@@ -16,6 +16,58 @@ tr.exportTo('tr.v.d', function() {
     tr.v.d.RelatedValueMap.call(this);
   }
 
+  /**
+   * Build a Composition and its NumericValues from |events|. Group events using
+   * |categoryForEvent|. Add the NumericValues to |values|. NumericValues' names
+   * are prefixed with |namePrefix|. Numerics are built by |numericBuilder|. The
+   * Numeric sample for each Event is derived from |opt_sampleForEvent|, which
+   * defaults to event.cpuSelfTime. The caller must add the result Composition
+   * to their Value's diagnostics.
+   *
+   * Composition encapsulates a generic relationship between NumericValues that

[charliea (OOO until 10-5), 2016/07/13 14:30:06]

Generally, the jsdoc describing what a type is goes along with the constructor.
Could you move the "Composition encapsulates a..." part of this comment up
there?

+   * can hold for groups of other things besides Events, such as memory

[charliea (OOO until 10-5), 2016/07/13 14:30:06]

I don't really have much context about Compositions here, but I'm not sure this
doc is parsable by people like me that lack context. I can't quite grok what "a
generic relationship between NumericValues that can hold for groups of other
things besides Events" means. Is it like a bunch of NumericValues that are all
associated somehow? Also, in this sentence, it says that it's a relationship
between groups of other things besides events, but in the next sentence, it says
that "composition over groups of events is expected to be the most common way of
building Compositions". Superficially, those two things seem at odds with each
other, but I'm pretty sure it's just because that lack of context.

Could you maybe give a more concrete example in the doc about how compositions
might be used to do something useful?

+   * allocations. Compositions over groups of Events is expected to be the most
+   * common way of building Compositions, though it is not the only way.
+   *
+   * @param {!tr.v.ValueSet} values
+   * @param {string} namePrefix

[nednguyen, 2016/07/13 02:17:32]

I still think this namePrefix is redundant. If people want this, they can just
modify their  categoryForEvent(e) function so that it returns <namePrefix> +
category(e) instead

[benjhayden, 2016/07/13 04:41:28]

The category isn't only used for the composing Value name, it's also used as the
Composition's key to the Value, which is displayed as a link in the
chart-base-legend-key.
So categoryForEvent returns something like 'style' or 'script', namePrefix is
something like 'long tasks ', so the composing value names are 'long tasks
style', etc., but it would be redundant to display 'long tasks style', 'long
tasks script', 'long tasks paint' in the legend-keys, not to mention they'd all
be elided at 'long tasks...'.

Alternatively, if the Value names are just the eventCategory strings with no
prefix, then there may be multiple Values named 'script', but one of them might
be for long tasks, another for loads, etc. but the Value names should indicate
that distinction. Hence namePrefix.

Does that help?

[nednguyen, 2016/07/13 11:34:02]

Hmhh, this makes sense but now I think about it, having value name & composition
legend being two totally different entities might cause unnecessary confusion
for people when it shows the composition's legend is "a b c" but when they click
on "a b c", it directs them to value named "d e f".

 Can we somehow enforce the legend name must be a suffix of value name? Probably
in a different CL though.

[benjhayden, 2016/07/13 18:03:04]

Sorry, I'm doing mega-CLs this week. :-)
This is a 2-line check-and-throw in Composition.set(), but it required changing
the default value of ChromeUserFriendlyCategoryDriver.fromEvent() from undefined
to 'unknown', FYI.

+   * @param {!tr.model.EventSet} events
+   * @param {!tr.v.NumericBuilder} numericBuilder
+   * @param {!function(!tr.model.Event):string} categoryForEvent
+   * @param {!function(!tr.model.Event):number=} opt_sampleForEvent
+   * @param {*=} opt_this
+   * @return {!Composition}
+   */
+  Composition.buildFromEvents = function(

[nednguyen, 2016/07/13 02:17:32]

s/buildFromEvents/buildFromEventCategories ? I don't have a strong opinion on
this naming though, so you can just keep the name the same.

+      values, namePrefix, events, numericBuilder, categoryForEvent,
+      opt_sampleForEvent, opt_this) {
+    var eventCallback = opt_sampleForEvent;
+    if (eventCallback === undefined) {
+      eventCallback = function(event) {

[charliea (OOO until 10-5), 2016/07/13 14:30:06]

nit: can be (event) => event.cpuSelfTime

+        return event.cpuSelfTime;
+      };
+    }
+
+    var composition = new Composition();
+    for (var event of events) {
+      var sample = eventCallback.call(opt_this, event);
+      if (sample === undefined)
+        continue;
+
+      var eventCategory = categoryForEvent.call(opt_this, event);
+      var value = composition.get(eventCategory);
+      if (value === undefined) {
+        value = new tr.v.NumericValue(
+            namePrefix + eventCategory, numericBuilder.build());
+        values.addValue(value);
+        composition.set(eventCategory, value);
+      }
+
+      value.numeric.add(sample, new tr.v.d.RelatedEventSet([event]));
+    }
+    return composition;
+  };
+
   Composition.prototype = {
     __proto__: tr.v.d.RelatedValueMap.prototype,