Perfomance issues. "add" method

[losogudok]

Hi, I've done some benchmarks and realized that script bottleneck is "add" function. We can make it work faster!

First thing to do, is to group all new elements in virtual DOM (document fragment) and then insert them together. DOM operations are expensive.

for (var i = childNodes.length - 1; i >= 0; --i) {
    var child = childNodes[i];
    if (!child) return;
    insertAfter(self.element, child, referenceElement);
  }

I will try to solve this problem.

[brianmhunt]

Awesome! Great feedback - look forward to seeing what you come up with!

[cervengoc]

Has any progress been made in this direction so far?

I also would be happy for this kind of improvement. The idea seems to be simple, and it's easy to gracefully degrade in older browsers by just simply using the original technic instead of document fragments.

Groupable items should be easily gathered together because based on my tests KO organizes the array change notifications in the correct order (tested with push with multiple arguments, and splice with multiple inserts).

I can try to put together a PR if noone has made it yet.

[brianmhunt]

@cervengoc I'd really love and appreciate a PR on this – I don't recall @losogudok sharing any progress, but @losogudok please let us know if you have. :)

Cheers

[AdamWillden]

@cervengoc I'd be of the assumption you might as well look into doing a PR yourself as it's been a while. If you have the time to get a PR together then I'd appreciate it too (I'd certainly benefit from it). I briefly looked at this issue but time isn't on my side at the moment - never is :(

[cervengoc]

@brianmhunt @AdamWillden yes, I've already started working on it, and the first results on a modern Chrome are stunning. Rendering time of 100-200 items in an array reduced to about 10%.

I have technical difficoulties with contribution though, to be honest I've never used GitHub in this way yet. Is there any kind of docs where it's explain what to install, how to run tests, etc.? I would really appreciate it.

[4ver]

@cervengoc Can you create a fork of this repo and push your changes to it? I'd love to have a look!

Prerequisites are having nodejs/npm and gulp installed. To install gulp run npm install gulp -g. To install this repos dependencies: npm install. To run the tests: npm test 😄 simple as!

[cervengoc]

@4ver Thanks, sure, I'll do it tomorrow

[brianmhunt]

@cervengoc @4ver Owing to my oversight, npm test will not work – you can test it with gulp live– then you can connect to the local webserver (the address should be printed when run). Sorry about that! 😁

[cervengoc]

I've made a fork and a commit with my solution, anyone feel free to check it. I'd happy to hear your thoughts and ideas. All existing tests are passed, I didn't create any new though, because I have no idea how this enhancement could be tested.

[brianmhunt]

Thanks @cervengoc !

It's hard to tell what's happening -- if you look at the diff it looks like almost every line changed (which does not actually seem to be the case).

Also it looks like you added the UMD headers to index.js – they should not go there, as the UMD headers are added automatically during the build process when you run gulp js. Sorry the code wasn't very clear on this. :(

Is there any way you can clean up the changes so the diff simplifies down to the salient bits?

When you've cleaned it up you can compare and start a Pull Request.

Cheers @cervengoc !

[cervengoc]

@brianmhunt I'll try to clean it up. Sorry, I don't have any experiance here yet. Also, I'm using visual Studio which probably auto-formatted the code, and that can lead to such heavy change log.

[cervengoc]

@brianmhunt OK, I reverted the previous commit, and made a new one, it seems to be much better. Please check it out.

[brianmhunt]

Ok, I've cleaned up the repo a bit and merged the PR as v0.4.0

There's one issue that lingers, namely: the line https://github.com/brianmhunt/knockout-fast-foreach/blob/master/index.js#L236 causes a failure in PhantomJS 1.X (the preferred browser for testing on CircleCI). This is an older Webkit browser, so lots of things break and I am ok not supporting it, but it would be good if the auto-tests passed.

I have tried the karma-phantomjs2-launcher which uses the newer PhantomJS 2, but it fails for no apparent reason.

Opening a new issue, but closing this one. Thanks @cervengoc and all.