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..415ce6b3527b546c18a702463a00696e7d83cd14 100644
--- a/tracing/tracing/value/diagnostics/composition.html
+++ b/tracing/tracing/value/diagnostics/composition.html
@@ -16,6 +16,53 @@ 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.

[benjhayden, 2016/07/12 17:29:57]

Clarify that Compositions can be built over sets of other kinds of things, such
as processes, threads, v8 samples, etc.

+   *
+   * @param {!tr.v.ValueSet} values

[nednguyen, 2016/07/12 17:46:24]

Why having values in the param and not just let the callsite add this to value
set themselves?

[benjhayden, 2016/07/12 22:59:47]

It seemed like it would be burdensome and error-prone to require callers to
iterate over the returned Composition and manually add the composing
NumericValues to the ValueSet.

+   * @param {string} namePrefix

[nednguyen, 2016/07/12 17:46:24]

I think we don't need this namePrefix since all of these values are generated as
a composition for existing value.

[benjhayden, 2016/07/12 22:59:47]

I suppose this function could take the parent Value and the name of the new
Composition diagnostic, and automatically add the new Composition diagnostic to
the parent Value's diagnostics, and automatically name the composing
NumericValues after the parent Value, but that seems like an unnecessary
restriction. Some metrics might want to use different naming schemes, different
name separators (space vs colon vs dash). Explicit is better than implicit.
General is better than special, though too general requires another layer of
tools.

+   * @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.build = function(

[eakuefner, 2016/07/12 16:18:48]

I'm a little confused -- why do you not simply put this code in the constructor?

+      values, namePrefix, events, numericBuilder, categoryForEvent,
+      opt_sampleForEvent, opt_this) {
+    var eventCallback = opt_sampleForEvent;
+    if (eventCallback === undefined) {
+      eventCallback = function(event) {
+        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,