Support Ember 2.0+ - Githubissues

GavinJoyce commented 9 years ago

There are also a number of existing performance tests which raise exceptions with 2.0.2. I'm fixing these in https://github.com/eviltrout/ember-performance/pull/52:

[x] link-to get('create')
[x] object-create/view
[x] Render List
[x] Render List (Unbound)
[x] Render Complex List
[x] Render Simple Ember List
[x] Render List with link-to
[x] Render link-to

rwjblue commented 9 years ago

Awesome! Thank you for working on this!

GavinJoyce commented 9 years ago

@rwjblue In the absence of ContainerView, what primitives do you think should I use for tests such as https://github.com/eviltrout/ember-performance/blob/master/tests/render-list/index.js?

I'm considering a creating an application with a template with a single if statement containing a component with the template to test. The test would flip the if to render the component and measure the time until afterRender. This doesn't seem like the same test though, rerenders may not exercise the code paths like they currently do. Are there lower level primitives that I should be using instead? Ideally I want to create a new component with some internal data and then append it.

It would also be nice if the updated tests to run in 1.11-1.13 and perhaps use public APIs so that they will work in future versions too

GavinJoyce commented 9 years ago

gavinjoyce [2:45 PM] 
I'm looking to update some of the `ember-performance` tests to support Ember 2.0 (edited)

gavinjoyce [2:46 PM]
Does anyone have any ideas how I should do this: https://github.com/eviltrout/ember-performance/blob/master/tests/render-list/index.js#L54-L59 (edited)

gavinjoyce [2:46 PM]
Now that `ContainerView` has been removed?

gavinjoyce [2:46 PM]
ie. how should I programmatically add a component?

mixonic [2:49 PM] 
@gavinjoyce: `{{#each componentNames as |componentName|}}{{component componentName}}{{/each}}`

mixonic [2:50 PM]
hm

mixonic [2:50 PM]
actually for that

mixonic [2:50 PM]
instead of a containerview

mixonic [2:50 PM]
just use a template with `{{#if componentName}}{{component componentName someArg=thing}}{{/if}}`

gavinjoyce [2:53 PM] 
thanks

gavinjoyce [2:53 PM]
to measure multiple times what should I do?

gavinjoyce [2:53 PM]
we're currently creating a new view and appending it each time

gavinjoyce [2:53 PM]
is there a way to create a component and append it?

gavinjoyce [2:54 PM]
if I just use templates I don't think it's quite measuring the same thing

mixonic [3:02 PM] 
@gavinjoyce: one sec

mixonic [3:11 PM] 
@gavinjoyce: so a template in Glimmer is really the only way to programmatically append/remove a component

mixonic [3:12 PM]
I guess "programmatic" is maybe not correct

mixonic [3:12 PM]
but it is the only way to append and remove one

mixonic [3:12 PM]
there isn't anything like containerview or collectionview any more

mixonic [3:13 PM]
IMO `{{#if foo}}{{my-component bar=baz}}{{/if}}` is appropriate if you want to test the rendering time of a single component

mixonic [3:13 PM]
anything else would need to use the instrumenting hooks

mixonic [3:13 PM]
https://github.com/emberjs/ember.js/pull/12088 (edited)

mixonic [3:13 PM]
^ perhaps tied to this

gavinjoyce [3:13 PM] 
in that case, should I toggle the `foo` to cause the component to be removed and added again?

gavinjoyce [3:14 PM]
will we end up mostly testing rerender in that case?

gavinjoyce [3:14 PM]
and not the initial render?

mixonic [3:15 PM] 
it would be testing the initial render of that component

gavinjoyce [3:15 PM] 
perhaps there is some internal state that I can remove so that rerender is more like the initial render?

mixonic [3:15 PM] 
and a rerender of that template

gavinjoyce [3:15 PM] 
ok

mixonic [3:15 PM] 
but the rerender of that template is effectively free

mixonic [3:15 PM]
hm

gavinjoyce [3:15 PM] 
yeah, that's what I'm hoping to avoid

mixonic [3:15 PM] 
I mean there should be an internal thing

mixonic [3:16 PM]
but I'm unsure how we would access it from user-land code

gavinjoyce [3:16 PM] 
initial render is a little slow right now and it would be good if the tests would capture that (edited)

mixonic [3:16 PM] 
ah

mixonic [3:16 PM]
yeah that would be a different test than what you linked to?

mixonic [3:16 PM]
that test does test initial render of a list

gavinjoyce [3:16 PM] 
that test does capture the slowness of initial render in 1.13 though

gavinjoyce [3:17 PM]
oh, sorry

gavinjoyce [3:17 PM]
that one is just representative

gavinjoyce [3:17 PM]
`Render Complex List` is a better one

mixonic [3:17 PM] 
so you could measure initial render time of a whole app

mixonic [3:18 PM]
but using a property toggle helps make the benchmark more specific to template rendering

gavinjoyce [3:18 PM] 
right, so I should probably create some new tests which focus on initial render

mixonic [3:18 PM] 
`{{#if foo}}some expensive stuff{{//if}}` <- this would test "initial render" of some expensive stuff

gavinjoyce [3:18 PM] 
and they can do something more expensive like create a new app each time

gavinjoyce [3:19 PM]
`{{#if foo}}some expensive stuff{{//if}}` that would tests the initial render just once though, even if we run that test 100 times

gavinjoyce [3:19 PM]
(I think)

mixonic [3:19 PM] 
that subtree would be initially rendered over and over

mixonic [3:19 PM]
the glimmer savings are regarding the top level template (that whole string)

mixonic [3:20 PM]
well, depending on the contents of `{{if` I suppose. As long as you do `{{#if foo}}{{my-component}}{{/if}}` you will measure initial render of `{{my-component}}` over and over

mixonic [3:21 PM]
glimmer throws away the render nodes of the subtree when `if` becomes false

gavinjoyce [3:21 PM] 
ok, thanks - I'll do some experimentation with this

mixonic [3:21 PM] 
it doesn't cache them
the efficiency of Glimmer is that when you rerender the top template, but `foo` is unchanged, zero work would be done

gavinjoyce [3:22 PM] 
right, thanks

mixonic [3:22 PM] 
if you toggle `foo`, work

rwjblue commented 9 years ago

Take a look at https://github.com/ef4/initial-render-perf for a representative way to do the complex list in 1.12 - ~2.2 (canary).

Key snippets:

Building a list of objects: https://github.com/ef4/initial-render-perf/blob/master/app/controllers/application.js#L3-L16
Toggle showing/hiding the list: https://github.com/ef4/initial-render-perf/blob/master/app/templates/application.hbs#L27-L29
Rendering the list with nested components: https://github.com/ef4/initial-render-perf/blob/master/app/templates/components/complex-items.hbs

GavinJoyce commented 9 years ago

Thanks, that's very useful

GavinJoyce commented 9 years ago

I've implemented a new RenderTemplateTestClient which allows for simple tests such as:

(function() {
  var template = "hi {{name}}";

  RenderTemplateTestClient.run({
    name: 'Render simple attribute',

    setup: function() {
      this.setupTemplateTest(template, { name: 'Ben' });
    },

    reset: function() {
      this.hideComponent();
    },

    test: function() {
      this.showComponent();
    }
  });
})();

This currently works with Ember 1.11+, are we happy to drop support for earlier versions? If not, it should be easy to add support for earlier versions in RenderTemplateTestClient

GavinJoyce commented 9 years ago

Here's the before and after of the tests (except the complex list which I have yet to fix):

Before

After

GavinJoyce commented 9 years ago

@rwjblue The tests are now running on 2.0: https://github.com/eviltrout/ember-performance/pull/52

GavinJoyce commented 9 years ago

Are we happy to drop support for Ember 1.10 and below? If not, we'll have to maintain multiple templates for things that have been removed like bind-attr

GavinJoyce commented 9 years ago

I've gathered results from before and after https://github.com/eviltrout/ember-performance/pull/52 to ensure that we're still capturing the slowdown and subsequent speed improvements in initial render performance:

https://docs.google.com/spreadsheets/d/15MPfzCncD_PvM-nmfdrA-_UsUzNpAVV9pFkt3wU5rBo/edit?usp=sharing

The new tests seem to be capturing this correctly (master on the left, https://github.com/eviltrout/ember-performance/pull/52 on the right):

rwjblue commented 9 years ago

Are we happy to drop support for Ember 1.10 and below? If not, we'll have to maintain multiple templates for things that have been removed like bind-attr.

@GavinJoyce - Yes, we can tag current master (to allow us to get back to where we are now if we want to do relative comparisons across many versions), but I definitely think we need to structure this for Ember 2.x. :+1: to dropping Ember 1.10 support.

eviltrout / ember-performance

Support Ember 2.0+ #51