Add Heimdall support

[oligriffiths]

This PR adds heimdall stats support to Broccoli master.

This is ported from https://github.com/ember-cli/broccoli-builder
I've tried to setup the test in as close a representation to the original as possible.

What we're doing is registering the beginNode and endNode events to start/stop the heimdall timer, but these events are no longer "nested" as they are in Broccoli < 1.0. As a result, to get the nested structure that heimdall needs to do flame graphs, etc, we need to reconstruct this graph using the Broccoli node graph.

Additionally, because Broccoli re-uses nodes (for source directories), and heimdall does NOT allow you to have a node with multiple parents, we're simulating a new heimdall node, in the same way as we're doing it in Broccoli < 1.0 https://github.com/ember-cli/broccoli-builder/blob/0-18-x/lib/builder.js#L101-L107 => https://github.com/broccolijs/broccoli/pull/362/files#diff-dde50c51d91de11d407f53423f3baeecR399]]]

Here's a vis output (running BROCCOLI_VIZ=1 ember build) for broccoli 1.0 and current master for 2 builds:

I used ember-cli/broccoli-builder#27 for the broccoli-builder branch as this makes the labels more informative and closer to broccoli (which I think is good).

full - a regular build of ember on a fresh application
slim - a simple build (2 nodes and a merge)

Archive.zip

These can be uploaded to https://rwjblue.github.io/heimdalljs-visualizer

They produce the following results:

broccoli-full:

master-full:

broccoli-slim:

master-slim:

As you can see, both flame graphs are the same, and broccoli-1 is slightly faster

Note

There appears to be a bug in the ordering of the output in the flame graph, looking into it...

[stefanpenner]

let use mocha's built in promise support, rather then done. This way, error handling is handled for us by default in tests.

This looks like:

it('foo', function() {
  return promise.then(() => {});
});

[oligriffiths]

Cool. Will do.

[oligriffiths]

@stefanpenner any idea how we can get the nested data for heidmall. Seems like it needs to be:

start(a)
start(b)
stop(b)
stop(a)

In order to make B a child of A.

Currently the node event system does

Start a
Stop a
Start b
Stop b

[stefanpenner]

broccoli 1.0 no longer evaluates nodes recursively. Rather it essentially top-sorts the plugin graph ahead of time, and produces a linear array of nodeWrappers, related code:

broccoli/lib/builder.js

Lines 144 to 146 in 7d97eba

	// This method recursively traverses the node graph and returns a nodeWrapper.
	// The nodeWrapper graph parallels the node graph 1:1.
	makeNodeWrapper(node, _stack) {

This nodeWrappers array is traversed, emitting the events between invocations of build, not as a parent of the recursive pattern from before.

What I am saying, is. The new pattern in broccoli is much nicer, but it does lose the parent/child hierarchy we had before, and thus may require a difference.

What likely could be done, is to emit as we do now. But ensure the start of a node, includes an array of ids for its inputs. Thoughts?

[oligriffiths]

Yes that was the conclusion I came to. I think adding the parent ids to the entry and reconstructing the recursive output would be a good idea. I also notice that in the broccoli-builder implementation, the total time for each node is the self time plus the sum of the inputs, which we should also preserve. This would mean iterating the heidmall tree from top down. I can work this out.

[stefanpenner]

Yes that was the conclusion I came to. I think adding the parent ids to the entry and reconstructing the recursive output would be a good idea. I also notice that in the broccoli-builder implementation, the total time for each node is the self time plus the sum of the inputs, which we should also preserve. This would mean iterating the heidmall tree from top down. I can work this out.

👍 1

@rwjblue can you confirm? This checks out for me.

[stefanpenner]

@oligriffiths also if at anypoint I'm blocking some aspect of broccoli work, feel free to pester me via any medium until I unblock you.

[oligriffiths]

@stefanpenner Super. Thanks

[stefanpenner]

Its a bummer we have to do this, but as part of this step I don't see a good option. I do believe as part of the 1x series, we will re-invest in heimdall, to make it and broccoli better friends.

[krisselden]

I actually find the current thing to be confusing because it makes the first parent look responsible for time it wasn't responsible for.

[oligriffiths]

@krisselden yeah I agreee. Heimdall is not able to properly model the broccoli graph. The fact that broccoli-builder would make “fake” nodes in order to satisfy shared plugins shows that the model is incorrect.

Unfortunately heimdall would require a decent rewrite to support this, or something else entirely.

[krisselden]

Not sure why it would model the broccoli graph, Id rather put inputBroccoli ids alongside broccoliId. Heimdal represnts what you’d put into a flame chart.

I hate that the visualizer now overweights the consuming node, it’s misleading.

[oligriffiths]

@krisselden a flame graph can’t accurately model a broccoli tree due to the fact that nodes are shared between inputs. Source nodes for example are reused. Even broccoli-builder had this same issue.