Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing instrumentation for compilation/lookup phase #15085

Merged
merged 4 commits into from
Apr 5, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions packages/ember-glimmer/lib/environment.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { guidFor, OWNER } from 'ember-utils';
import { Cache } from 'ember-metal';
import { Cache, _instrumentStart } from 'ember-metal';
import { assert, warn } from 'ember-debug';
import { DEBUG } from 'ember-env-flags';
import { EMBER_NO_DOUBLE_EXTEND } from 'ember/features';
Expand Down Expand Up @@ -56,6 +56,10 @@ import { FACTORY_FOR } from 'container';

import { default as ActionModifierManager } from './modifiers/action';

function instrumentationPayload(name) {
return { object: `component:${name}` };
}

export default class Environment extends GlimmerEnvironment {
static create(options) {
return new Environment(options);
Expand Down Expand Up @@ -147,11 +151,17 @@ export default class Environment extends GlimmerEnvironment {

getComponentDefinition(path, symbolTable) {
let name = path[0];
let finalizer = _instrumentStart('render.compile', instrumentationPayload, name);
let blockMeta = symbolTable.getMeta();
let owner = blockMeta.owner;
let source = blockMeta.moduleName && `template:${blockMeta.moduleName}`;

return this._definitionCache.get({ name, source, owner });
let definition = this._definitionCache.get({ name, source, owner });
if (definition) {
definition.finalizer = finalizer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mixonic notes that this may not work as expected if the same component is used multiple times.

} else {
finalizer();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear that two control flows (with and without definition) are being weighed the same way. @rwjblue suggests always calling the finalizer at the end of the is method, and changing the instrumentation identifier to something more specific like 'render.getComponentDefinition' and then add other separate instrumentations for other parts of the code.

return definition;
}

// normally templates should be exported at the proper module name
Expand Down
3 changes: 3 additions & 0 deletions packages/ember-glimmer/lib/syntax/curly-component.js
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,8 @@ class CurlyComponentManager extends AbstractManager {
}

create(environment, definition, args, dynamicScope, callerSelfRef, hasBlock) {
definition.finalizer();

if (DEBUG) {
this._pushToDebugStack(`component:${definition.name}`, environment)
}
Expand Down Expand Up @@ -421,6 +423,7 @@ export class CurlyComponentDefinition extends ComponentDefinition {
super(name, MANAGER, ComponentClass);
this.template = template;
this.args = args;
this.finalizer = () => {};
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
import { moduleFor, RenderingTest } from '../../utils/test-case';
import { Component } from '../../utils/helpers';
import {
instrumentationSubscribe,
instrumentationReset,
set
} from 'ember-metal';

moduleFor('Components compile instrumentation', class extends RenderingTest {
constructor() {
super();

this.resetEvents();

instrumentationSubscribe('render.compile', {
before: (name, timestamp, payload) => {
if (payload.view !== this.component) {
this.actual.before.push(payload);
}
},
after: (name, timestamp, payload) => {
if (payload.view !== this.component) {
this.actual.after.push(payload);
}
}
});
}

resetEvents() {
this.expected = {
before: [],
after: []
};

this.actual = {
before: [],
after: []
};
}

teardown() {
this.assert.deepEqual(this.actual.before, [], 'No unexpected events (before)');
this.assert.deepEqual(this.actual.after, [], 'No unexpected events (after)');
super.teardown();
instrumentationReset();
}

['@test it should only receive an instrumentation event for initial render'](assert) {
let testCase = this;

let BaseClass = Component.extend({
tagName: '',

willRender() {
testCase.expected.before.push(this);
testCase.expected.after.unshift(this);
}
});

this.registerComponent('x-bar', {
template: '[x-bar: {{bar}}]',
ComponentClass: BaseClass.extend()
});

this.render(`[-top-level: {{foo}}] {{x-bar bar=bar}}`, {
foo: 'foo', bar: 'bar'
});

this.assertText('[-top-level: foo] [x-bar: bar]');

this.assertEvents('after initial render');

this.runTask(() => this.rerender());

this.assertEvents('after no-op rerender');
}

assertEvents(label) {
let { actual, expected } = this;
this.assert.strictEqual(actual.before.length, actual.after.length, `${label}: before and after callbacks should be balanced`);

this._assertEvents(`${label} (before):`, actual.before, expected.before);
this._assertEvents(`${label} (after):`, actual.before, expected.before);

this.resetEvents();
}

_assertEvents(label, actual, expected) {
this.assert.equal(actual.length, expected.length, `${label}: expected ${expected.length} and got ${actual.length}`);

actual.forEach((payload, i) => this.assertPayload(payload, expected[i]));
}

assertPayload(payload, component) {
this.assert.equal(payload.object, component._debugContainerKey, 'payload.object');
}
});