[Perf] Array property changes not being coalesced (for reduceComputed?)

[SohumB]

See this jsbin.

The source array changes often and in large batches. We rely on array property change coalescing for this to not be a performance issue.

The dependent items array has elements that each have a property dep, that is dependent on some property of the full items array - in this case, length. Then we have an {{#each}} that iterates over these dependent values.

All of this behaves fine, so far.

This breaks down, however, when you add an Em.computed.max on the resulting items.dep values. Click the button, and see how many times the length property gets called.

In our codebase, the equivalent of length is an expensive operation that we really would have wanted and expected to run with coalesced changes, once per runloop. The expected flow is: items is generated, with no value for each item.dep yet, then length is computed once, then the items.dep values are computed, then the max property is computed. But Ember appears to be recomputing max and thus forcing recomputes of everything upto length on every array change operation.

Thanks!

[simonexmachina]

Any thoughts on this issue?

[ahacking]

@aexmachina This may be a dup/related to my own reduceComputed bug #9313. There is also a meta issue #5268 and PR #9356 and PR #9492.

Whilst #9356 provides improvements to address the way arrayComputed/reduceComputed handles some corner cases I think there is a realisation that a different approach (sort and merge/diff) is required. Sadly current project deliverables leaves me no time to invest in such an alternative solution as much I would like to.

[simonexmachina]

@ahacking yeah, it does like like AC/RC are in need of an overhaul. I don't suppose you've got any tips for a workaround to avoid the thrashing? We might be able to use Ember.run.once() but then the CP becomes async.

[ahacking]

Yes I have used Ember.run.once() to avoid repeated updates as well. The deferred/async update wasn't an issue in my use cases but its more soothing an ailment than treating the core problem.

As per the issues I referenced, a potential way forward is to use a stable merge sort derivative and then diff the before and after state calling add/remove functions on the CP as appropriate. I don't think the core of it is actually much work compared with the complexity there currently, but I just don't have the luxury of time to bite off polishing and testing a full Array CP replacement at this stage. I did look into various sorting algorithms and implementations and the upshot was that tsort appears to be best algorithm for array based CP's as it can exploit runs, its stable (doesn't reorder items otherwise considered equal) and would be close to O(n) for minor changes as is the case with UI interactions.

There may be other approaches to array CP's but given the complexity of what's there currently its obvious there are many edge cases. It would be nice to elegantly bypass the problem areas as I hope the sort/diff approach could do. Perhaps a dirty list or bitmap might allow more efficient CP updating. @ef4 would certainly have a better understanding and may be able to elaborate since he has been into the depths of array CPs, at least for the sorting use case.

[simonexmachina]

@SohumB this might be a potential workaround for us. Also, @SohumB do you have any thoughts on @ahacking's discussion above.

Conceptually it should be possible to use arrayObservers to produce a set of transformations that can then be fed through the AC/RC interface in a way that behaves like a computed property (where changes are coalesced), but the current implementation seems to attempt to do this and doesn't really succeed. I agree that a diffing algorithm could well be a simpler approach.

[ef4]

I think regenerating and diffing is the right idea. Ultimately we'd like the diffing to happen at the DOM level, so you can manipulate the list any way you want (including a total regeneration) and still get minimal DOM updates. There's lot of active discussion around this, and the current HTMLbars work lays some of the groundwork to make it possible.

[SohumB]

Yeah, I agree that diffing at the DOM level would be a whole lot more preferable. Given how small the arrays tend to be for UI elements, I have to wonder how far a "just recompute everything" strategy would be able to take us - which would bypass all of the fragility and incidental complexity here. I suspect that in the cases that diffing would matter for performance, your data's coming in as diffs anyway - or at least can be translated to diffs at some layer with more semantic information about the changes involved.

I'd also like to raise that reduceComputed has an incredibly weird design at the moment - it's really meant for incremental reducers, ones you can update with just the diff information. changeMeta.arrayChanged does provide the full array, but even though property changes are coalesced, Ember will call your addedItem and removedItem handlers multiple times, so you can't realistically do an O(n) pass inside there.

[stefanpenner]

I have to wonder how far a "just recompute everything" strategy would be able to take us

as react has demonstrated, quite far.

[SohumB]

React also couples that with a very efficient DOM diffing engine, so it's not quite the same comparison until the new htmlbars stuff comes in. I meant now, with whatever DOM thrashing is involved.

[simonexmachina]

"Just recompute everything" + coalescing property changes should avoid the DOM thrashing.

[stefanpenner]

React also couples that with a very efficient DOM diffing engine, so it's not quite the same comparison until the new htmlbars stuff comes in. I meant now, with whatever DOM thrashing is involved.

its gets us far

please note that ember's KVO system is very well tuned for these types of updates. Unfortunately when an entire array changes, it forces a entire re-render. If we don't change the array, but mutate it, ember's underlying system handles this scenario optimally. Using diffing at this point provides clear wins, as it restores not minimal re-rendering goal ember has, even with naive array re-building.

The point of array/reduce computed shifts the problem of preserving array equality into a large complex book keeping problem. The book keeping itself current is both expensive and in many scenarios flawed. If we can remove this limitation we get a big win.

[ahacking]

@SohumB I agree the reduceComputed interface is mighty strange and difficult to work with. There is no such thing as a change event just add/remove.

I think for now its probably easier and cleaner to just observe the array and use a Ember.run.once to recompute your derived value as currently reduceComputed is broken and does excessive recomputation over the entire array on a change anyway.

[simonexmachina]

I've gotta say that IMHO the current implementation should be moved out to a plugin, and eventually replaced with something that's not "kinda broken".

[ef4]

Ripping out the implementation we have would be a breaking change, so it would not be something to rush into.

But people who want a new implementation right away could definitely do it as a plugin, and that would be fantastic.

On Tue, Dec 2, 2014 at 9:06 PM, Simon Wade notifications@github.com wrote:

I've gotta say that IMHO the current implementation should be moved out to a plugin, and eventually replaced with something that's not "kinda broken".

On Wed, 3 Dec 2014 13:03 ahacking notifications@github.com wrote:

@SohumB https://github.com/SohumB I agree the reduceComputed interface is mighty strange and difficult to work with. There is no such thing as a change event just add/remove.

I think for now its probably easier and cleaner to just observe the array and use a Ember.run.once to recompute your derived value as currently reduceComputed is broken and does excessive recomputation over the entire array on a change anyway.

— Reply to this email directly or view it on GitHub https://github.com/emberjs/ember.js/issues/9739#issuecomment-65340655.

— Reply to this email directly or view it on GitHub https://github.com/emberjs/ember.js/issues/9739#issuecomment-65341017.

[ahacking]

@aexmachina I agree, along with you and others no doubt have wasted a significant amount of time persuing an approach based on array cp working and what looks like a first class feature in the framework only to find out its not really there in practice. I'm fine with unstable features if they are marked as such but the array cp stuff is in such a state that it really should be feature flagged as experimental since its not at the level it should be. I am not being critical or unappreciative of the hard work people have put into this feature, more about the curation process and how features like array cp get promoted to stable ember release given the problems.

[stefanpenner]

more about the curation process and how features like array cp get promoted to stable ember release given the problems.

unfortunately this in inherited from a time before our strict release + go/no-go policy :(

[rwjblue]

I am not being critical or unappreciative of the hard work people have put into this feature, more about the curation process and how features like array cp get promoted to stable ember release given the problems.

It landed just before 1.0.0 and therefore before we instituted the feature flagging and general feature review processes.

[ahacking]

@stefanpenner Thanks, I'm not intimately familiar with Ember change history so I was surprised given all the good work I have observed recently. I'm glad of the current strict policy :)

[ahacking]

@rwjblue thanks. I might PR an Ember documentation change then to give people a heads up then.

[simonexmachina]

Just to add to this issue, Sohum and I spent several hours chasing down a bug coming from #each and eventually tracked it down to another AC issue. This fix was this:

-  visibleItems: Em.computed.filterBy('wrappedItems', 'onCurrentPage')
+  visibleItems: function() {
+    return this.get('wrappedItems').filterBy('onCurrentPage', true);
+  }.property('wrappedItems.[]', 'wrappedItems.@each.onCurrentPage')

You can imagine we were pretty unimpressed by this. I think it really goes to show that this really shouldn't be included in Ember until it's reliable. We're two very experienced Ember developers and so we were able to chase this down, but anyone with less experience would be completely flummoxed by this.

Another exciting thing we came across in our travels was the following code in reduce_computed.js (in DependentArraysObserver#flushChanges):

this.setValue(
  this.callbacks.removedItem.call(this.instanceMeta.context, this.getValue(), c.obj, changeMeta, this.instanceMeta.sugarMeta));
this.setValue(
  this.callbacks.addedItem.call(this.instanceMeta.context, this.getValue(), c.obj, changeMeta, this.instanceMeta.sugarMeta));

Call removedItem(), then addedItem() with the exact same arguments, for... reasons? We were debugging and noticed that a view in our #each was being destroyed, when there had been no corresponding removal in the underlying array. Turns out that, because of this code above, that's normal behaviour - sometimes the views are just destroyed and then recreated for no apparent reason. Is this perhaps an example of the broken AC/RC leaking into another layer of the system?

Ideally we'd be able to remove this because it's obviously sub-optimal from a performance POV, but it's also quite confusing from a debugging point of view. Should I create an issue for this?

[simonexmachina]

For anyone interested, here's some very simple implementations that we use to get around the problems with ArrayComputed and ReduceComputed: https://gist.github.com/aexmachina/cb49e3a396c6c35e276c.

These use a naive approach of traversing the whole array every time it changes, and they work just fine. Performance doesn't seem to be much worse for small arrays (< 100):

= Ember
29938
33873
33733
33587
33194

= our naïve replacements
38093
35559
34868
33245
33716

[SohumB]

diff --git a/modules/attic/addon/utils/unbork-arraycomputed-reducecomputed.js b/modules/attic/addon/utils/unbork-arraycomputed-reducecomputed.js
index 241901b..2abb7e9 100644
--- a/modules/attic/addon/utils/unbork-arraycomputed-reducecomputed.js
+++ b/modules/attic/addon/utils/unbork-arraycomputed-reducecomputed.js
@@ -26,4 +26,20 @@ Em.computed.filterBy = function(key, property, value) {
   });
 };

+Em.computed.sort = function(key, sortDefinition) { // yes, it rolls up sort and sortBy
+  Ember.assert('Ember.computed.sort requires two arguments: an array key to sort and ' +
+               'either a sort properties key or sort function', arguments.length === 2);
+
+  var sortProperty = typeof sortDefinition === 'string';
+  var dep = sortProperty ? key + '.@each.' + sortDefinition : key + '.[]';
+
+  return Em.computed(dep, function() {
+    if (sortProperty) {
+      return Em.get(this, key).sortBy(sortDefinition);
+    } else {
+      return Em.get(this, key).sort(sortDefinition);
+    }
+  });
+};
+

Yeah, I absolutely and utterly think arrayComputed and reduceComputed need to be removed.

[simonexmachina]

@igorT another one! ^

[ahacking]

For the record the complexity inherent in CPs, array CPs and dealing with a plethora of feedback related issues that come with observers made me realise the Ember approach needs to be questioned.

Added to the fact that this ties in with how views are rendered and the DOM is updated there is also constant friction/no-can-do with view context/scope in handlebars/htmlbars. The expressivity in handlebars/htmlbars requires a lot of CPs since templates only provide a limited language to express some basic JS constructs with #if, else and #each and yield where whats in scope is a real problem. There is no ability to reference a components properties/CPs from a yield, or wire up components or their properties from handlebars (eg a video player and its controls). The new block params work and bikeshedding over syntax and the how and what should be passed as block params, or what should be in context by default has led me to question the ENTIRE Ember paradigm and look elsewhere.

I've come to the conclusion that a React/Flux style approach offers a MUCH cleaner way forward for both client and server SEO/rendering.

Why?

• No need for the complexity of CPs
• I can use fast immutable datastructures and get very strong guarantees of my applications behavior
• I can use immutable data and get undo/redo changesets for free
• I get damn fast rendering and low overhead processing because I don't need to express state in terms if CPs and all of the overhead that entails.
• I can use real JS for each, map, reduce, if, else etc and not be limited to constructs handlebars provides
• I can know exactly what is in scope in my templates.
• no limited expressivity as is the case with handlebars. I can easily map/reduce the data I need into components.
• I can use real JS in my {}'s if using JSX so that my entire view is composed of components and the stuff in {} uses JS scope rules!!
• React has a damn fine router too modelled on Embers thanks to other Emberists who also looked outside the box.
• There are just far less surprises and gotchas

I never thought I'd be saying this because so much in Ember looked so good going in, but getting a polished app across the line with Ember has taken its toll.

[stefanpenner]

@ahacking I believe you have identified all the things we agree on and are working towards. Hopefully soon you will see the continued evolution satisfy these concerns. It's unfortunate we don't satisfy these points today.

One thing I take offense to is the "open minded" statement. Ember has evolved from a desktop like experience on the web(sproutcore) to a solution that tries to embrace the web. Almost annually ember has reinvented and refined it self to not only improve the status quo but remove entire categories of problems. We are not afraid of change, rather attempt to embrace it and allow the system to evolve as the web ecosystem learns.

It is unfortunate that the acceleration of this evolution can't be instantaneous. I personally wish it was. Ultimately everything requires some elbow grease, time, reinvestment and perseverance.

I have enjoyed your too the point and often brutally honest comments and mostly hold these thoughts in high regard. I am sad to see your opinion of our open mindedness as negative as it is. :(

[ahacking]

@stefanpenner Sorry if I have offended, I didn't believe I made a statement about "open mindedness". I am certainly aware of Embers roots and the constant evolution it has undergone by the tireless efforts of the core team including yourself. I have immense respect for what Ember core has done, and I regard you all as leaders in contemporary web development.

My previous statement was not intended to criticize Ember developers and community or even suggest Ember can't re-invent itself. My statement was for the record a candid list of some of what I encountered as critical aspects that have been the "cause for pause" and the drivers for considering alternatives. I could silently walk away but I felt it was important to state why. Ember is still the most complete OSS web framework and has a holistic view not found anywhere else, I still have Ember projects to maintain so I'm far from done with Ember.

However I do believe that React/Flux for what it offers is a better proposition both architecturally and technically as well as from a developer productivity/maintenance/robustness/fragility point of view. These things are hard to discern, you only really understand the value and merit of approaches after you have used them in anger. I had previously used other frameworks which "drove" me to Ember and now I am being driven to something else, even though as I said, so much looks so good in Ember.

I was initially excited with the prospect of htmlbars (and it is great stuff), but it is still entrenched in the logic-less template model which requires helpers and lots of CPs, as well as a constrained expressibility when composing the UI. This is Embers way of trying to tame unnecessary computation and minimal DOM updates, but the approach starts to get strained dealing with scopes in templates, property observer feedback, CP's or observers that have a number of inputs or are mutually dependant and fire multiple times and so on. These are problems that React JSX templates don't have; if I want to reduce a set of numbers and divide by another to present a percentage I just do it, no CP/observer shenanigans and dealing with deferred calculations and the run loop. Better yet, if I use immutable datatypes I avoid ALL unnecessary re-computation and get undo/redo for free in a way that is incredibly empowering compared to what Ember requires.

The React approach doesn't require declarative templates in order to minimize DOM updates, and doesn't need to defer to CP's and helpers, it just has components that can render their data and virtual DOM diff. This approach I feel is a critical factor in developer productivity, deterministic outcomes and clean/robust implementations that are maintainable over the long haul.

React doesn't have the coverage of Ember, but it allows a superior architecture which works well on both client and server. Its not all encompassing like Ember and Ember CLI. and I have to pull in a router, immutable datatypes, a flux implementation, a data layer and cobble the build system together. Ember offers a lot of turn-key value that React doesn't yet.

In closing I feel Ember is at a local maximum, and requires a leap to another curve to break free of its legacy which I see are CPs and handlebars templates. I see Ember re-inventing itself with htmlbars and a push to components, deprecating controllers, server side rendering and "services" (which are like pink unicorns from where I sit) but I'm not seeing the kind of moves that change the game like Flux/React does. If the Ember core team was to embrace the Flux unidirectional architecture approach (Action -> Dispatch -> Stores -> View Controller -> DOM update) and a paradigm built around immutable datatypes and DOM diff I'd be rallying behind you all 100%.

[simonexmachina]

@ahacking, totally respect your decision but for my part Ember is still providing exactly what I'm looking for, and I think the plans for 2.0 brings the benefit of lessons learnt from React. Ultimately Ember still delivers the coverage, workflow and conventions that I need for my team.

I am a bit horrified at this dirty little corner of Ember's codebase, and I've made it clear that I think it should be removed. For the time being we're just implementing simple replacements as we go (which I'm hoping to release soon).

I think React's got a lot to offer but I don't believe for a second that it's a panacea, and while it may be the ultimate for me or you, that doesn't mean that it's going to be the same for everyone else on my team. React is awesome, but they've got a long way to go to get anywhere near Ember, and I'm sure there'll be people looking to jump ship to the next shiny new thing once the limitations of their approach become well understood.

@ahacking, did you end up submitting a PR to add a warning to the docs? @stefanpenner would you be open to a PR that says "ArrayComputed and ReduceComputed can exhibit unexpected behaviour in some circumstances. Some people in the Ember community recommend that you use alternative implementations such as ..." It would be good to have "official" replacements that are maintained by the community.

@stefanpenner, are there any plans for resolving this in Ember 2.0? People keep getting mauled by this, even @IgorT, and he's a certified Ember ninja.

[ahacking]

@aexmachina

No I didn't submit a PR, I just put forth the proposal but didn't get a positive indication that it would be accepted.

I mostly agree with everything you say. Also please don't get me wrong I also recognise React is only a small part of an overall solution and has nowhere near the coverage that Ember has. Its not a case of shiny new things for me, quite the contrary. I'm more interested in the Flux architecture pattern, immutable data and dom diff as a way foward independent of any particular framework. Its just that React has managed to capture that ecosystem an those approaches seem diametrically opposed to rendering with handlebars, CPs and observers. As much as I do like Ember and what the core team are doing holistically over any other framework including React I recognise that I am probably more aligned with the approach React allows currently.

It would be great to have a clearer idea of the future direction for Ember 2.0 as I don't like to assume what 2.0 means. @stefanpenner has indicated here there are big changes ahead for 2.0, its just not clear what that overall vision is even though I do know the Ember core are pretty big thinkers in this space. I know that "services" are coming, controllers are dead and long live components but that doesn't convey enough of the intent and overall architecture vision. I'm kindly asking for more detail please, and if there's nothing public then please publish something that clearly articulates and defines the 2.0 architecture and how data and events flow in that architecture.

[ef4]

The Road to 2.0 RFC is pretty explicit about the high-level goal. It explicitly calls out React-like data flow. On Dec 22, 2014 8:04 PM, "Stefan Penner" notifications@github.com wrote:

I'm kindly asking for more detail please, and if there's nothing public then please publish something that clearly articulates and defines the 2.0 architecture and how data and events flow in that architecture.

@tomdale https://github.com/tomdale @wycats https://github.com/wycats

— Reply to this email directly or view it on GitHub https://github.com/emberjs/ember.js/issues/9739#issuecomment-67911235.

[simonexmachina]

@ef4 I agree. @ahacking have you read the RFC?

[ahacking]

@ef4 @aexmachina I wasnt aware of that RFC but I've since read it now thanks.

Whilst the RFC points out some important changes I still don't get a sense of the big picture and how all those changes together transform the Ember architecture. I just don't see it, despite the fact every change seems like a reasonable one.

I see words 'react-like' but that doesn't mean a lot. I really I want to see a holistic picture of data and event flows within the 2.0 architecture and just exactly how you compose everything together as its not clear to me that I can actually compose components effectively with the proposed RFC. I don't see block params as sufficient; What if I have an accordion and want separate blocks/components for the header and body/pane? There are a few basic use cases that I can't see working and which I raised on the block params rfc.

I think a diagram showing all event and data flows in the 2.0/woorld view would really help to focus and align the proposed changes and validate that they do in fact support the goal.

I also don't see a react/flux like single directional flow architecture in the 2.0 rfc despite 2.0 changing how component actions will work. I don't see a clear possibility of immutable data being supported in the 2.0 RFC. These may well be possible or may not be a goal on purpose, but its not clear without a high level architecture overview.

I've heard "services" talked about but no mention in the RFC. Are these intended to replace models and data source/persistence layer?

Anyway these are questions/observations I should raise over on the 2.0 RFC.

[machty]

I don't see block params as sufficient; What if I have an accordion and want separate blocks/components for the header and body/pane? There are a few basic use cases that I can't see working and which I raised on the block params rfc.

@ahacking a blockParam.content-for helper would handle this pretty elegantly.

[ahacking]

@machty thanks that sounds like it could work.

I'm keen to see all these things that make composition workable in some kind of overview given htmlbars has such a limited vocabulary compared to say a Dom builder api or JSX with scope preserving embedded js.

This sounds like a core feature to support composition but I didn't see it in the 2.0 RFC.

I have to admit I love the ability to filter/map with real js as per React for composing components in either js or jsx so I'm seeing handlebars and helper escape hatches as a bit of a legacy/round-about way to get things done. I see value in Ember having a clean Dom builder api so you have a choice of using htmlbars style templates, pure js or something like JSX but I didn't see something like that within the 2.0 RFC.

[machty]

@ahacking I love that about JSX as well, but there are tradeoffs; once you start using maps, filters, or any sort of iteration in React, you've got to pull that mapping fn out of the main JSX return value, or if you do inline it all in the return, you wind up with something like

  <ul>
    {
      this.props.people.map(function(p) {
        return <li className="person">Hello {p.name}</li>;
      })
    }
  </ul>

I've experimented with React in a section of our app where immutable structures were desirable (though I already had it working with some fidgeting in Ember) and it immediately became the part of the app that my HTML/CSS designer/dev could no longer touch. I know this isn't everyone's story, but there are real tradeoffs that both React and Ember fans are still feeling out.

Certain aspects of HTMLBars could be better / more elegantly expressed in JSX, but the same could be said the other way around (such as the above example): Handlebars/HTMLBars is attempting to be the best possible presentation language on the block while still being faithful to HTML and relieving designers of the need to understand what a closure is; JSX is attempting to minimize the number of technologies (and unnecessary coupling between them) to build out web app view layers, while still kinda resembling HTML, but assumes that you definitely must know JavaScript to do anything meaningful with it.

[ahacking]

@machty I think that's an excellent description of the trade-offs. Personally I like the code you show and it looks even nicer with ES6 fat arrows.

<ul>
  {this.props.people.map(p => <li className="person">Hello {p.name}</li>)}
</ul>

Or using a PersonItem component:

<ul>{ this.props.people.map(p => <PersonItem data={p}/>) }</ul>

I accept different shops will have different responsibilities and work flows. I just lean more towards developer happiness with a change request if the mark-up doesn't work for the styles the designer wants to apply. You end up having to do this for structural changes anyway as new templates partials etc may be introduced and I don't expect designers to understand anything within moustaches, bind-attrs, where to put outlets, Ember template naming conventions and so on.

I think both our comments demonstrate the base rendering layer should be amenable to both, hence my earlier suggestion that a foundational DOM building api similar to reacts or potentially whatever htmlbars uses internally would be a less opinionated and IMO a more sensible thing. Compiled htmlbars/handlebars style templates or any other templating system (including things like JSX) could then use the DOM builder api.

[simonexmachina]

So just wanted to return this fascinating conversation to the original issue: what should we do about this bear-trap lurking in the Ember codebase? Is there a plan of action or should I PR a warning in the docs for this feature.

[ef4]

I'd favor a PR deprecating arrayComputed and reduceComputed. People who want to keep using them could be directed to an add-on.

This would be a warning only for now, with removal at 2.0.

[simonexmachina]

We have simple replacements that are not broken too, but they map/reduce over the whole array on each change.

On Wed, 7 Jan 2015 17:17 Edward Faulkner notifications@github.com wrote:

I'd favor a PR deprecating arrayComputed and reduceComputed. People who want to keep using them could be directed to an add-on.

This would be a warning only for now, with removal at 2.0.

— Reply to this email directly or view it on GitHub https://github.com/emberjs/ember.js/issues/9739#issuecomment-68983426.

[patricklx]

hey, i extracted RPC into an addon with some changes to fix various issues i had. if anyone is interested: https://github.com/patricklx/ember-cli-reduce-computed