Heimdall instrumentation #4510

runspired · 2016-08-24T04:45:57Z

This is a WIP branch that allows us to easily measure where our performance costs are in different record loading scenarios.

Example:

runspired · 2016-08-24T04:49:38Z

The .json files generated by the instrumentation runner should be added to git-ignore, that's the bulk of the line changes. Will prune those out in a moment

stefanpenner · 2016-08-24T15:51:07Z

Awesome. I suspect we may (once this matures) shake out pieces into their own repo's and modules, but keeping it together during experimentation seems legit.

fivetanley · 2016-08-25T13:59:16Z

beat me to it. nice!

fivetanley · 2016-08-25T14:00:33Z

it would be great to allow instrumentation from the outside. on example would be to add some instrumentation for "how many models/requests were loaded during this route transition", but that's probably a separate thing i could implement

(sorry thinking aloud in github comments :P)

runspired · 2016-08-25T14:24:40Z

I made some progress on reducing the cost of the instrumentation itself, so we may be able to leave some of it in for people if they want and use babel transforms to strip the rest of it out for production builds.

runspired · 2016-08-29T18:13:07Z

Results of a recent round of measurements are below, I'm currently working on enabling ED to strip out heimdall instrumentation for production builds.

hjdivad · 2016-08-29T20:04:00Z

@fivetanley

it would be great to allow instrumentation from the outside. on example would be to add some instrumentation for "how many models/requests were loaded during this route transition", but that's probably a separate thing i could implement

This is an excellent suggestion and, unless I've misunderstood you, also something heimdalljs explicitly supports. This is how we use it to track fs usage within ember-cli for instance.

see:

stefanpenner

yay cleanup

runspired · 2016-09-14T22:53:38Z

Getting happier, I've squashed and rebased. Got more instrumentation tweaks to make, need to push out an alpha release for heimdall-query and the heimdal babel transform, and figure out what is breaking tests.

runspired · 2016-09-15T05:43:40Z

@igorT not sure why this test is failing, could use some help tracking it down.

runspired · 2016-09-15T05:48:27Z

lib/babel-build.js

@@ -18,6 +18,7 @@ function babelOptions(libraryName, _options) {
      'es6.constants',
      'es6.modules'
    ],
+    externalHelpers: true,


this is from implementing a plugin to strip classCallCheck while investigating alternative adapters, it should be removed.

runspired · 2016-09-15T05:50:03Z

package.json

@@ -47,6 +48,7 @@
  "devDependencies": {
    "bower": "^1.6.5",
    "broccoli-asset-rev": "^2.1.2",
+    "broccoli-babel-transpiler": "^5.6.1",


this creeping in here was accidental, will be removed.

runspired · 2016-09-15T05:50:57Z

package.json

    "loader.js": "^4.0.0",
+    "lodash": "^4.15.0",


I'm not sure lodash is directly needed anymore, I'll do a quick review to make sure. I believe this was only here for what is now heimdall-query.

runspired · 2016-09-15T05:51:59Z

tests/dummy/app/helpers/reopen-instrumentation.js

@@ -0,0 +1,51 @@
+/* global heimdall */


despite setting this globally many places ESLint was yelling at me if I didn't set it at the top of files :/ This should be worked out.

runspired · 2016-09-15T05:53:01Z

tests/dummy/app/routes/query/route.js

+        heimdall.stop(token);
+        // console.log('ember-data-end', stats.endTime);
+        // console.profileEnd(label);
+        // console.info('findAll#item -> fetched ' + get(records, 'length') + ' records');


all these console call comments should be removed.

runspired

Almost ready for PR to be reviewed for merging.

stefanpenner · 2016-09-15T15:09:23Z

addon/-private/system/model/internal-model.js

@@ -44,6 +44,24 @@ function retrieveFromCurrentState(key) {
 }

 var guid = 0;
+
+const [
+  __a, __b, __c, __d, __e, __f, __g, __h, __i,


we should use more descriptive names?

fivetanley · 2016-09-15T15:16:13Z

i think a small facade layer between heimdall and ember data would be nice. this allows us to document it as part of the Ember Data API and also change the implementation more easily if we needed to in the future.

runspired · 2016-09-15T15:48:40Z

@fivetanley Describe what you are thinking. I had no intentions of the instrumentation being public beyond a user being able to leave it in their build and provide their own heimdall instance if desired.

stefanpenner · 2016-09-15T18:43:48Z

@fivetanley I believe the public API you seek is heimdall itself. As it aims to provide cross cutting instrumentation (Ember/loader/ember-data/custom app code) etc.

rwjblue · 2016-09-15T18:51:36Z

I believe the public API you seek is heimdall itself.

Does it have a public API? I didn't think it was stable yet (don't see a 1.x or anything)...

runspired · 2016-09-15T18:53:55Z

@rwjblue we're narrowing in on it, 0.3.x is close to stable

runspired · 2016-09-15T18:55:46Z

@rwjblue and fwiw, the public API in heimdall 0.3.x-alpha3 is at this point highly unlikely to change (the last thing likely to change was the switch over from returning an array to returning an object when calling registerMonitor which I just made this morning), we're mostly playing around with internals at this point.

fivetanley · 2016-09-15T19:04:21Z

Is this PR for benchmarking, or is it desired to also be useful to end users?

stefanpenner · 2016-09-15T19:06:38Z

Is this PR for benchmarking, or is it desired to also be useful to end users?

first benchmarking, later developers caring about performance (maybe the extension and tests etc.)

runspired · 2016-09-15T19:07:35Z

@fivetanley this PR is part of a concerted effort to have consistent instrumentation across ember-cli, ember, glimmer2, and ember-data to help us more quickly identify performance problems and regressions.

Benchmarking is secondary, but nice. One of the awesome things about this approach is that our instrumentation cost is low enough that it can be left in for end users when they desire, but easily stripped out for the primary case. Soon an end user wanting in depth measurements can select to not strip heimdall and provide their own heimdall instance for use.

runspired · 2016-09-15T18:58:27Z

addon/-private/system/model/internal-model.js

+  'send',
+  'transitionTo',
+  '_triggerDeferredTriggers'
+);


@stefanpenner style preference here? Expanded like this or moved onto fewer lines?

chadhietala · 2016-09-15T20:11:01Z

@runspired I'm going to kill some CI jobs that you have running that are clogging up the concurrency.

runspired · 2016-09-16T22:31:45Z

@igorT code review cleanup is now complete.

bmac · 2016-09-19T12:47:30Z

Overall this pr looks good to me @runspired. 👍

fivetanley

Looks good but a few things need addressing. Also instructions for using/running heimdall need to be in the README before merging.

fivetanley · 2016-09-19T13:41:49Z

lib/stripped-build-plugins.js

+
+  if (environment === 'development') {
+    // uncomment when running baseline development benchmarks
+    // plugins.push(stripHeimdall);


would prefer an environment variable here. inevitably this comment to not commit it will be skipped or forgotten or not seen in a code review.

fivetanley · 2016-09-19T13:41:58Z

lib/stripped-build-plugins.js

+      }),
+      // comment out when running non-baseline production benchmarks
+      // WARNING do not ever commit the commented out version!
+      stripHeimdall


would prefer an environment variable here. inevitably this comment to not commit it will be skipped or not seen in a code review.

@fivetanley I've found it very hard to replicate a production build without using the production env, if you have suggestions I'm open. Was going to look into a process flag to use as a toggle to keep us safe.

production builds are issued on CI requests and tags auto publish to bower. I wouldn't want heimdall to get in there accidentally; historically build related changes have been difficult to track down and reproduce. something like process.env.ENABLE_HEIMDALL would prevent this change from accidentally being checked into source control.

Was going to look into a process flag to use as a toggle to keep us safe.

Agreed ;)

runspired · 2016-09-23T01:12:58Z

This should be good now, Travis tests will pass I think appveryor was just a fluke.

cc @fivetanley I added a bit of documentation to CONTRIBUTING.md, and --instrument as a flag must be used to turn instrumentation on, it's always off and stripped otherwise.

cc @HeroicEric the counters are now alphabetized :)

fivetanley · 2016-09-23T03:01:23Z

we'll need to double check with @tricknotes and other globals-mode users to ensure this works in gems/bower. it looks great from a build perspective but just trying to be careful since we tend to have build-related regressions whenever we change this stuff.

runspired · 2016-09-29T19:59:32Z

@igor passing :)

bmac · 2016-09-30T18:32:19Z

@runspired do you mind rebasing this pr?

runspired · 2016-09-30T18:33:44Z

@bmac I don't but did you know you can rebase and squash from the merge button now? 💃

…marking

runspired · 2016-09-30T18:35:39Z

@bmac rebased :)

fivetanley · 2016-09-30T18:46:28Z

@tricknotes do you have some cycles so we can make sure this still works in the globals version of ember data?

bmac · 2016-09-30T18:54:10Z

@fivetanley I'd like to merge this pr into the master branch. I can cut an alpha gem release to make testing easer and we will have plenty of time to fix any regressions because these changes will have to go through a full beta cycle. Does that sound good to you?

fivetanley · 2016-09-30T18:58:17Z

Sounds good to me. I'll open a tracking issue.

tricknotes · 2016-09-30T22:35:43Z

@fivetanley Sorry to my late :<
Now I confirmed global builds, and it seems to work fine for me 🚀

runspired force-pushed the heimdall-instrumentation branch from 8e3b420 to 360d272 Compare September 12, 2016 01:42

stefanpenner reviewed Sep 14, 2016

View reviewed changes

runspired force-pushed the heimdall-instrumentation branch from e25a653 to f5dc86b Compare September 14, 2016 21:33

runspired commented Sep 15, 2016

View reviewed changes

stefanpenner reviewed Sep 15, 2016

View reviewed changes

runspired commented Sep 15, 2016

View reviewed changes

fivetanley requested changes Sep 19, 2016

View reviewed changes

runspired force-pushed the heimdall-instrumentation branch from e60ca71 to 4b3b32b Compare September 29, 2016 19:43

runspired added 10 commits September 30, 2016 11:35

adds a mock server, dummy app, and heimdall instrumentation for bench…

d3966fa

…marking

fix benchmarking for latest ember-cli

5b046ba

safer toggling of instrumentation

86bb1d0

adds note on benchmarking to the readme

e390bd9

improve benchmarks and instrumentation

e8f425d

bump dep

bdf1ac9

alphabetize counters

0c8022f

dont import heimdall unless instrumentation is needed

0c99e46

fix env variable

b49e4ee

safe merge filtered imports

7a1a957

runspired force-pushed the heimdall-instrumentation branch from 4b3b32b to 7a1a957 Compare September 30, 2016 18:35

fivetanley approved these changes Sep 30, 2016

View reviewed changes

fivetanley merged commit 2340426 into emberjs:master Sep 30, 2016

wecc removed concept-review Tracks PRs that introduce new concepts that need reviewed for discussion team-review labels Oct 20, 2016

runspired deleted the heimdall-instrumentation branch October 21, 2016 16:37

Heimdall instrumentation #4510

Heimdall instrumentation #4510

Conversation

runspired commented Aug 24, 2016

runspired commented Aug 24, 2016

stefanpenner commented Aug 24, 2016

fivetanley commented Aug 25, 2016

fivetanley commented Aug 25, 2016

runspired commented Aug 25, 2016

runspired commented Aug 29, 2016

hjdivad commented Aug 29, 2016

stefanpenner left a comment

Choose a reason for hiding this comment

runspired commented Sep 14, 2016

runspired commented Sep 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runspired Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

runspired left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetanley commented Sep 15, 2016

runspired commented Sep 15, 2016

stefanpenner commented Sep 15, 2016 • edited Loading

rwjblue commented Sep 15, 2016

runspired commented Sep 15, 2016

runspired commented Sep 15, 2016

fivetanley commented Sep 15, 2016

stefanpenner commented Sep 15, 2016 • edited Loading

runspired commented Sep 15, 2016

Choose a reason for hiding this comment

chadhietala commented Sep 15, 2016

runspired commented Sep 16, 2016

bmac commented Sep 19, 2016

fivetanley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runspired commented Sep 23, 2016

fivetanley commented Sep 23, 2016

runspired commented Sep 29, 2016

bmac commented Sep 30, 2016

runspired commented Sep 30, 2016

runspired commented Sep 30, 2016

fivetanley commented Sep 30, 2016

bmac commented Sep 30, 2016

fivetanley commented Sep 30, 2016

tricknotes commented Sep 30, 2016

runspired Sep 15, 2016 •

edited

Loading

stefanpenner commented Sep 15, 2016 •

edited

Loading

stefanpenner commented Sep 15, 2016 •

edited

Loading