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

Array.observe() + sort() ==> event flooding #16

Open
about-code opened this issue Feb 7, 2015 · 4 comments
Open

Array.observe() + sort() ==> event flooding #16

about-code opened this issue Feb 7, 2015 · 4 comments

Comments

@about-code
Copy link

Array.observe() should be specified in more detail with respect to sorting an array. In recent versions of Chrome the code snippnet below indicates that its current implementation of Array.observe doesn't scale well when it comes to sorting. There are a lot more change records generated than there are elements in the array. As it seems there's an update for each permutation. I think there should be just a single "update" change record after sorting, where "name" refers to a pseudo property 'order'.
Example:
{
type: 'update',
name: 'order',
object: [/sorted (orig. reference)/],
oldValue: [/* unsorted (reference to clone?)*/]
}

// i = 9 => changes.length = 54
// i = 9999 => changes.length = 143499
var x = [];
for (i = 9; i >= 0; i-=1) {
x.push(i);
}
Array.observe(x, function (changes) {
console.log(changes)
});
x.sort(function (e1, e2) {
if (e1 < e2) {
return - 1;
}else if(e1 > e2) {
return 1;
} else {
return 0;
}
});

@about-code
Copy link
Author

Maybe only a single update change record is a bit radical in the opposite direction and not that smart either. Of course, its the developers who have to be concious here, and the browser vendors who have to protect end users from those who are not, similar to how they do with infinite loops and other resource limits. Maybe should see it this way. Sorry for bothering you.

@arv
Copy link
Owner

arv commented Feb 9, 2015

I agree.

One clean record would be the mapping from old index to new index.

@rafaelw
Copy link
Collaborator

rafaelw commented Feb 9, 2015

FWIW, when we thought about this problem in the past, what prevented us from specifying something here was the lack of certainty which of the following cases may occur:

  1. sort() results in only minor modifications to array
  2. sort() results in major modification to the array

In case (2) emitting a single "splice" event with the entire state of the array prior to the sort, is the most efficient thing to do.

However, in the case of (1), if the array is large, that has the potential to be needlessly expensive (i.e. a few individual property changes would have been preferable)

@about-code
Copy link
Author

Thank you both for your feedback.

@rafaelw:
Am I understanding you correctly, if I say, the "conflict" you describe is that sorting an array is a major modification comprised of many minor modifications?

I want to adopt that view. Object/Array.observe is often discussed together with data-binding. When it comes to DOM manipulation we usually want to avoid unnecessary re-renderings (reflows). When sorting a data-bound array, observers are probably more interested in the major modification, that is they want to get notified once for all changes to element positions rather than about every single (minor) update during sorting. So I think @arv's proposal would be more convenient here, since it would be a single record, which not just allowed to recompute the old array state, if needed, but to get those positions in particular, which changed. So we could precisely update the parts of the DOM which are associated with the elements that moved. That being said, there are of course valid scenarios for when observers also want to listen for every minor change that occurs during sorting (just thinking of visualizations for sort algorithms ;-). The point here is, that as a result of sorting

  • I would like to be able to listen for one major change record as well as for all minor change records
  • I would like to be able to distinct between major change record and minor change record by their type.

I am not sure whether Issue #17 opened by @alexanderby refers to a similar need. However the 'move' event type he proposes is interesting and may be a way to communicate (minor) position changes while I would prefer @arv's proposal for an 'update' event which communicates major change made by sort().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants