bennadel / JavaScript-Demos

A collection of online demos created from blog posts.
763 stars 467 forks source link

I don't think you are comparing apples to apples. #3

Open mhevery opened 8 years ago

mhevery commented 8 years ago

The issue is that your generateGrid (https://github.com/bennadel/JavaScript-Demos/blob/master/demos/render-large-datasets-angular2-reactjs/angular.htm#L238) method creates new set of objects each time it runs. Angular 2 then has to remove the DOM and recreate all of the items for the purposes of animation. So the Angular perf test is doing a lot more then it should.

There are two ways to fix it:

  1. change gerenerateGrid to return an array with same identities.
  2. change *ngFor (https://github.com/bennadel/JavaScript-Demos/blob/master/demos/render-large-datasets-angular2-reactjs/angular.htm#L88) to use trackBy See: https://github.com/angular/angular/blob/master/modules/angular2/src/common/directives/ng_for.ts#L86 (It will be in beta4)

Tho above should make Angular significantly faster.

More reading: https://github.com/mathieuancelin/js-repaint-perfs/issues/19

bennadel commented 8 years ago

@mhevery I very much appreciate the feedback - I know you guys are super busy. But, just to be clear, my intent was to look at a few different things:

  1. Mounting / unmounting the grid specifically to test DOM destruction and creation.
  2. Highlighting cells by dynamically altering the class-names.
  3. Reversing the order of the DOM to see how existing DOM items were moved based on existing object identities.

For the generateGrid(), I am re-generating the objects, but only when I actually unmount / remount the grid. Notice that when I updating the filtering (to change the class names), I am mutating the grid in-place, so the DOM should be unaffected.

That said, I do think that reversing the grid becomes very problematic because I am creating new object references:

https://github.com/bennadel/JavaScript-Demos/blob/master/demos/render-large-datasets-angular2-reactjs/angular.htm#L196-L213

.... I am re-creating ROW references, which is probably destroying and recreating every single TR. That was as mistake on my part. Really, I should have just called .reverse() on the grid, and .reverse() on each set of grid items.

As far as TrackBy, it looks like it already landed in Beta 3 - I was just looking at it over the weekend:

http://www.bennadel.com/blog/3020-understanding-object-identity-with-ngfor-loops-in-angular-2-beta-3.htm

.... but, I think I maintain all the object references, the trackBy becomes less relevant and I'll just rely on Object Identity. I'll make those updates and report back.

As an aside, I am really loving Angular 2 - it's definitely a lot to learn, but I actually really enjoy the new syntax. Really, it's not so different and there's not really that much to learn (about the syntax). I think it provides nice clarity.

Much appreciated!

mhevery commented 8 years ago

Glad to hear, please update me, would love to hear the progress.

Also, how do you make sure that the same DOM movements happen in React?

bennadel commented 8 years ago

@mhevery Ok, so fixing the reverse-grid algorithm, to keep the object identity references in tact, definitely sped up the rendering by like 30%. Beforehand, the reverse action was taking close to 300ms on average. Now, with the in-pace reverse with no DOM destruction (confirmed), it's close to the 200ms-230ms. So, clearly a big improvement.

The ReactJS rendering time is still a bit faster on the reverse, coming in around 160ms-180ms time. But, definitely very close to each other.

Video: http://www.screencast.com/t/uQq2irn3Y6v

In the video you can see that I include optional logging of the TR and TD elements to confirm that the DOM elements are not destroyed on the reverse sort. Here is the commit to ensure the in-place changes:

https://github.com/bennadel/JavaScript-Demos/commit/c32f3c5b6540e5a6727ffd852297662d27d9a7f9

Thanks for your help.

mhevery commented 8 years ago

I still don't see trackBy here: https://github.com/bennadel/JavaScript-Demos/blob/master/demos/render-large-datasets-angular2-reactjs/angular.htm#L99

Also still don't think you are comparing same things. In case of React the react just updates in place, whereas Angular is trying to do the right thing, and move things around. If you want to make the two benchmarks same, then you should change Angular to track by position (like React)

*ngFor="var item in items; trackBy: position" where position is function position(index, item) { return index; }. This way the two benchmarks will be identical.

mhevery commented 8 years ago

We could make position tracking the default, but I feel that in that case we would be great at benchmarks but would be wrong in real world, such as for animation (something which react can't do out of the box). So it seems wrong that we should be doing things to make ourselves look fast at the expense of correctness.

bennadel commented 8 years ago

I am not sure that you are correct about the in-place editing for React. I think that - by default - it will update in place. But, my understanding is that when you start using key={ ... } to link a DOM instance to the view-model, it will start moving things around. I am using key for both the:

TR: https://github.com/bennadel/JavaScript-Demos/blob/master/demos/render-large-datasets-angular2-reactjs/react.htm#L417

TD: https://github.com/bennadel/JavaScript-Demos/blob/master/demos/render-large-datasets-angular2-reactjs/react.htm#L471

From the React Documentation:

The situation gets more complicated when the children are shuffled around (as in search results) or if new components are added onto the front of the list (as in streams). In these cases where the identity and state of each child must be maintained across render passes, you can uniquely identify each child by assigning it a key ... When React reconciles the keyed children, it will ensure that any child with key will be reordered (instead of clobbered) or destroyed (instead of reused).

This is open to interpretation, but I think it means that React will move around the DOM instead of updating in place for the majority of the grid (my "ID" column doesn't have a key, so it probably is being updated in-place).

As far as the trackBy, I don't think I need it now that I am maintaining the object identities of the grid records. My trial and error would indicate that it is working. Also, when I stopped re-building the grid on "shuffle", the time to execute dropped by 1/3. I attribute this to the inherent tracking by object-identity.