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

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

Open
mhevery opened this issue Feb 10, 2016 · 6 comments
Open

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

mhevery opened this issue Feb 10, 2016 · 6 comments

Comments

@mhevery
Copy link

mhevery commented Feb 10, 2016

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: mathieuancelin/js-repaint-perfs#19

@bennadel
Copy link
Owner

@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
Copy link
Author

mhevery commented Feb 11, 2016

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
Copy link
Owner

@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:

c32f3c5

Thanks for your help.

@mhevery
Copy link
Author

mhevery commented Feb 13, 2016

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
Copy link
Author

mhevery commented Feb 13, 2016

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
Copy link
Owner

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.

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

2 participants