Use a queue instead of array reduce for performance #142
Conversation
|
Wow this is impressive! I'll have a look at this soon. Great find! |
|
checking to make sure we don't re-introduce this bug. there is a test, cool |
|
That's good to check and have tests for it. I tried to make other optimizations elsewhere and was surprised, but happy, that tests failed - helped me a lot in understanding how the code works. |
|
still checking the tests a little, I hit a case where the tests still pass when commenting out this whole block: I feel like there should be a test failing when we do this. // prevChildren.forEach(u => {
// if (update._children.indexOf(u) === -1) {
// u._fresh = true;
// }
// });
// // If any children were marked as fresh remove them from the run lists.
// let curr;
// const queue = [].concat(update.children);
// while (curr = queue.pop()) {
// if (curr._fresh) {
// curr._observables.forEach(o => {
// if (o._runObservers) o._runObservers.delete(curr);
// });
// }
// curr.children.forEach(u => queue.push(u));
// } |
|
Hmm yeah. That does seem strange. As for the actual PR changes though, it doesn't look like any observables could run or cause changes (to the run list or other observables for example) - I imagine deleting all children from run lists is safe regardless of the order of operations? |
|
you're right, just noticed those are deleted anyway I'm wondering if that whole block might be unneeded then because that case Ryan explained is passing its tests. |
| allChildren.forEach(removeFreshChildren); | ||
|
|
||
| let curr; | ||
| const queue = [].concat(update.children); |
There was a problem hiding this comment.
just noticed, missing the underscore here. update._children
There was a problem hiding this comment.
WHOA. Huh sorry I missed that converting from a TS codebase back to JS... I'm too used to the editor catching that :(
I was suspicious of the number of function calls in gathering children so I made a performance test: perf.link
Firefox is 19% the speed.
Opera/Blink is 1% (!!!) the speed.
