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

computed.sort slows down and hangs when sortProperties change #4712

Closed
RandomEtc opened this issue Apr 13, 2014 · 1 comment
Closed

computed.sort slows down and hangs when sortProperties change #4712

RandomEtc opened this issue Apr 13, 2014 · 1 comment
Assignees

Comments

@RandomEtc
Copy link

It seems that computed.sort can slow to a crawl and hang browsers if the sortProperty is changed too often.

Here's a minimal demo using computed.sort: http://jsbin.com/bagalowo/3/edit Click the table headers to re-order. After 10-15 times things get noticeably slow.

The TableController is pasted below for reference. The sort action on the table headers modifies the sortProperty which in turn is used be computed.sort to update the sortedRows. It works, but each time sortProperty changes, the internals of computed.sort take up more and more time.

Theory: it appears that the multiple calls to setupSortProperties.call(this); and the sortPropertyDefinitions.addObserver('@each', this, updateSortPropertiesOnce); line are interacting badly and updateSortPropertiesOnce is filling up the run-loop with redundant work? Here's the relevant part of 1.5 - https://github.com/emberjs/ember.js/blob/v1.5.0/packages/ember-runtime/lib/computed/reduce_computed_macros.js#L685 - I've confirmed this is also an issue in the latest canary, used in the above jsbin.

SortApp.TableController = Ember.Controller.extend({
  rows: [
    { name: 'Ian', age: 14 },
    { name: 'John', age: 12 },
    { name: 'Karen', age: 13},
    { name: 'Lisa', age: 10 },
    { name: 'Mike', age: 19 }
  ],
  sortedRows: Ember.computed.sort('rows','sortProperty'),
  sortProperty: Ember.A(['age:asc']),
  actions: {
    sort: function(property) {
      var sortProperty = this.get('sortProperty');
      var currentSort = sortProperty[0].split(':');
      if (currentSort[0] == property) {
        var dir = currentSort[1] == 'asc' ? 'desc' : 'asc';
        sortProperty[0] = property+':'+dir;
      } else {
        sortProperty[0] = property+':asc';
      }
      this.set('sortProperty', sortProperty);
    }
  }
});

cc @hjdivad @ebryn as follow-up from https://twitter.com/RandomEtc/status/455030469138997248

PS some workarounds: use a computed property to create an ArrayProxy with SortableMixin instead, or refactor to use an ArrayController with arrangedContent if possible.

@hjdivad hjdivad self-assigned this Apr 15, 2014
@wagenet wagenet added assert and removed assert labels Apr 15, 2014
@krisselden
Copy link
Contributor

closing as duplicate of #4740

mmun added a commit to mmun/ember.js that referenced this issue Apr 22, 2014
…y dependencies should be synchronous"

This reverts commit 7af8beb which attempted to
bring the semantics of ReduceComputedProperty closer to those of ComputedProperty
but inadvertently introduced a performance regression in some cases.

ComputedProperty guarantees that any `set` calls that occur before a `get` call
will be run before the `get` call is triggered. For example

```javascript
var obj = Ember.Object.extend({
  foo: Ember.computed.alias('bar')
}).create();

obj.set('foo', 1);
obj.get('foo'); // returns 1
```

For technical reasons ReduceComputedProperty does not offer the same
guarantees.

```javascript
var obj = Ember.Object.extend({
  filtered: Ember.computed.filter('things', function(n) {
    return n % 2 === this.get('parity');
  }).property('things', 'parity')
}).create({
  things: [0, 1, 2, 3, 4, 5]
});

o.set('parity', 0);
o.get('filtered'); // returns [0, 2, 4]

o.set('parity', 1);
o.get('filtered'); // returns [0, 2, 4], but we expected [1, 3, 5]

Ember.run.next(function() {
  o.get('filtered'); // returns [1, 3, 5]
});
```

The commit fixed this issue but introduced a performance regression. Every call
to `set` would eagerly force a recomputation of the reduced computed value,
instead of deferring the work until a subsequent `get` call. This was seen
most prominently in Ember.computed.sort with sortProperties that changed
quickly. You can find more details about the regression at:

emberjs#4712
emberjs#4740

Conflicts:
	packages_es6/ember-runtime/tests/computed/reduce_computed_test.js
rwjblue pushed a commit that referenced this issue Apr 23, 2014
…y dependencies should be synchronous"

This reverts commit 7af8beb which attempted to
bring the semantics of ReduceComputedProperty closer to those of ComputedProperty
but inadvertently introduced a performance regression in some cases.

ComputedProperty guarantees that any `set` calls that occur before a `get` call
will be run before the `get` call is triggered. For example

```javascript
var obj = Ember.Object.extend({
  foo: Ember.computed.alias('bar')
}).create();

obj.set('foo', 1);
obj.get('foo'); // returns 1
```

For technical reasons ReduceComputedProperty does not offer the same
guarantees.

```javascript
var obj = Ember.Object.extend({
  filtered: Ember.computed.filter('things', function(n) {
    return n % 2 === this.get('parity');
  }).property('things', 'parity')
}).create({
  things: [0, 1, 2, 3, 4, 5]
});

o.set('parity', 0);
o.get('filtered'); // returns [0, 2, 4]

o.set('parity', 1);
o.get('filtered'); // returns [0, 2, 4], but we expected [1, 3, 5]

Ember.run.next(function() {
  o.get('filtered'); // returns [1, 3, 5]
});
```

The commit fixed this issue but introduced a performance regression. Every call
to `set` would eagerly force a recomputation of the reduced computed value,
instead of deferring the work until a subsequent `get` call. This was seen
most prominently in Ember.computed.sort with sortProperties that changed
quickly. You can find more details about the regression at:

#4712
#4740

Conflicts:
	packages_es6/ember-runtime/tests/computed/reduce_computed_test.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants