Performance improvements

[krzysztof-grzybek]

Hi!

This is a preview PR of changes requested in #20514 Zendesk request.
I believe that the code will explain our intentions better than words 🙂
Just please keep in mind that this is just a preview of our intentions. Surely it needs some typing improvements, probably better structure and a rebase to the current master.

The content below is just a copy-paste from Zendesk request.

---

We use the viewport row model and at some point, we call params.setRowData function. This call takes ~200ms to complete, which causes glitches.

It turns out that the majority of this time is spent on parsing HTML. It comes mostly from the method getCreateTemplate. After changing this piece of code from assembling string template and calling loadTemplate to create the DOM manually using methods like document.createElement, element.setAttribute etc. we experienced 5 to 10 times faster execution of params.setRowData function.

We checked also the DEMO on the main ag-grid page and it seems that parsing HTML in function redrawAfterScroll takes around 18% of the time, so it seems that the issue is not case-specific.

With that being said, we request to switch from parsing HTML strings to creating the DOM with DOM API.

Please note, that we use ag-grid in version: 25.3.0, but the same method for creating the DOM i is present in the latest master branch on Github.

I'm attaching screens from the performance inspection.

Before the change:

After the change:

---

I created a plunker for that purpose - https://plnkr.co/edit/Tisqid5a4Im7lkQx?

To reproduce this issue, try to resize the screen to have many columns and rows visible at once, and then just scroll down the grid. Every hundred items scrolled, you should see a blank grid for a while. It's the time that takes javascript to populate the grid with the data.

[makinggoodsoftware]

Hi,

Thanks for submitting this PR, at the moment we don’t have a safe mechanism to automatically merge these, but we do this manually instead.

At this stage, your PR is being considered, we should update you as soon as possible, if this PR were to be approved note that we would do this in our end manually and we would close this, but you would get your changes effectively into our source code.

At the moment this is going to be flagged with ag-grid engaged

Thanks for your collaboration!

[krzysztof-grzybek]

Thank you Alberto!

[kiril-matev]

Hi Krzysztof!

Thanks very much for creating a PR with your changes!

We found that your PR is off a branch of AG Grid 25.3, which was released last year. In the future can you submit PRs using the latest version as a base instead?

The reason we say this is that Cell Comp code has changed a lot since AG Grid 25.3. In particular, the new Cell Comp no longer uses templates to the same extent it did before. The DOM is now built up mostly in JavaScript - this was part of the work we did to share logic with React UI in v26 and 27.

This is what our code looks like now in Cell Comp:
this.setTemplate(/* html */<div comp-id="${this.getCompId()}"/>);

Note the template is tiny, all the other bits are updated using JavaScript.

Can you please get our latest v27 release and see if applying your suggested change to this version produces a noticeable impact?

[krzysztof-grzybek]

Thanks for checking this Kiril,

Yes, I'm aware that this is PR to the previous version of ag-grid. It just meant to be a preview PR and I had this work already done.

Right, I can see that there less templating on the current master. However, I'm afraid that it might still affect the performance.
The reason I think this way is because even changing this innerHTML to createTextNode was a huge performance gain (surprisingly!).

But that sounds like a good idea. We'll update to the v27 and see how the performance will look like. I'm not sure when we'll do this, it might take a while. I will let you know.

[Harpush]

On ag grid 27 and 28 we are experiencing the same problem. Using createElement improves performance drastically. I spoke about it with @StephenCooper in discord too. Seems like ParseHTML is the culprit here even when using small templates. At a large amount of cells it adds up.

[krzysztof-grzybek]

@Harpush We upgraded to 28 recently and we're still experiencing the same issue. We're using a patched version of ag-grid and we see a 2-3 times performance boost for methods for the crucial tasks.

[kiril-matev]

Hi @krzysztof-grzybek,

Thanks for following up.

The viewport row model is an AG Grid Enterprise-only feature which requires a license giving you access to support.

In order to continue the earlier discussion we had about this, I've opened a support ticket in our internal support system (#25425) which is the most effective way we investigate issues in AG Grid Enterprise as they require collaboration on our side and makes it easier for us to track related work (bug/feature request) if needed.

However, if this investigation results in a bug/feature request being open, I will follow up here for the benefit of the AG Grid developer community.

In general, I would recommend that as an AG Grid Enterprise customer you submit bug reports in our internal support system to begin with to avoid delays in processing.

Also, when reporting issues please note:
1/ Clearly identifying the issue and not spending time trying to investigate its causes in AG Grid code, submit PRs or patch AG Grid code. Once we have clearly established there's an issue our developers will do all the necessary investigation and resolution.
2/ More often than not a PR cannot be readily integrated into our codebase because it is built to resolve the issue in your specific use case while we need to ensure that it works in a wider set of use cases without any impact on the remaining features of AG Grid.
3/ Patched versions of AG Grid cannot be supported by us as we only support the official version of the product.

We're looking forward to working with you to look into this further. Please respond to the support ticket I've opened in our system.

Just to reiterate for everyone else's benefit - if this investigation results in a bug/feature request being open, I will follow up here for the benefit of the AG Grid developer community.

[kiril-matev]

Just to clarify this further - I'm not able to reproduce the behavior you reported with the sample you provided. Please keep in mind that the fact that the grid spends a particular amount of time in one method or another is not indicative of an end-user rendering issue in and of itself. We'd like to see the end-user manifestation of any issue in order to investigate further.

I've updated your original plunker example as follows:
1/ Increased width of the left-most row id column to more clearly display the row id
2/ Increased amount of data threefold to ensure more scrolling space

However, I can't reproduce the issue you reported - the grid viewport was never completely blank for me when scrolling, it always showed the row background.

Please see the steps below and clarify. Please let me know if I need to use a different set of steps to reproduce this.

Steps to illustrate:

1. Open
   original sample with AG Grid v26.2 https://plnkr.co/edit/0ryb2ugXt0YaSzLQ
   sample with AG Grid v28.2.1 https://plnkr.co/edit/EhyxzPEyLmdzw8qi?open=app%2Fapp.component.ts
2. Open plunker preview in full screen using button just above preview pane (in order to show all columns and more rows)
3. Scroll grid rows down/up quickly (but not all the way to the bottom)
   Actual:
   1/ While scrolling occasionally grid rows are displayed with only values in the left-most column, with other cells getting rendered after a small delay
   2/ Row background is always shown
   3/ The grid viewport never shows a completely white screen

As you can see in the GIF below, there's no blank white screen as you report with either AG Grid v26.2 or v28.2.1.

Please let me know whether the GIF above shows the issue you're reporting or provide a different set of steps for me to follow to reproduce the issue you reported.

[Harpush]

@kiril-matev If I can add to it as we experience the same thing generally. On a good computer it might be neglectable but on a user's computer which isn't as good as dev's it will perform worse and have a meaning. I opened the plunkr and ran chrome performance test for first render on a not so good computer as there we see the most problems and I guess scrolling and first render are pretty close. In addition it is easier to check first render.
For the sake of the check I added a button that renders and hides the grid (using *ngIf) to accurately check performance.
The first render was indeed slow and a lot of time was spent on ParseHTML of templates.

Video of the first render:

Performance check bottleneck for the three biggest render parts (tasks):

The summary of all the render time:

[kiril-matev]

Hi @Harpush,

Thanks for looking into this further.

Can you please send me the example you're using so I'm sure to be looking at the exact same behavior you're seeing? So this isn't about getting a white screen when scrolling the grid.

As per your description, you're reporting as an issue the time it takes the grid to render the first time. Please confirm.
Also, can you confirm - you see no issue with the rendering speed when scrolling the grid?

[Harpush]

@kiril-matev Sure. Its exactly the same example as you sent with a button added. This is it anyway: https://plnkr.co/edit/o7QERYRVuyjzd3wl.
Concerning scroll performance I have the same behavior as you (rows render flicker but no "real" white screen). Not sure it is the desired behavior anyway as I would expect a more fluid virtual scroll. Here is a scrolling gif from the same plunkr I linked from above:

And indeed the problem I am describing is the first render being slow as seen in the previous comment's gif.

[Harpush]

@kiril-matev Just to add - I ran a performance check for scrolling to check if the flicker is for the same reason as first render. And I am pretty sure it has a large part in it:

This is a summary of scrolling the grid (similar to the gif from the previous comment). Loading (ParseHTML) has a noticeable effect here.

[kiril-matev]

Hi @Harpush,

Thanks for this response.

The example we've been using is using Angular and the viewport row model, as well as sizing columns to fit after a delay. All of these could be introducing additional delay when rendering.

If this is a rendering issue in AG Grid, it should be reproducible when using JavaScript and a simple client-side row model for the data.

Here are the steps I'm following:

1. Open
   https://plnkr.co/edit/OrSRm8HACFEOjRIy
2. Open plunker preview in full-screen
3. Click DESTROY GRID button to hide grid
4. Open console > PERFORMANCE tab
5. Throttle CPU speed by 4x
6. Start profiling
7. Click CREATE GRID button
   Actual: Grid briefly renders blank, then shows rows and cell values
8. Once grid renders fully, click STOP PROFILING
9. Look at the performance profile, the BOTTOM-UP tab
   Actual: ParseHTML takes up about 13% of the total time

This amount of time spent in ParseHTML I see in my testing is significantly less than the results you're getting. Can you reproduce this behavior using the JavaScript example and steps above?

Please note I'm using CPU throttling to emulate a slower machine so this effect would be more noticeable.

Please let me know.

[krzysztof-grzybek]

@kiril-matev I did some more detailed testing. After the upgrade, we can clearly see the performance boost in the devtools, but from the user perspective, it's hard to determine if it's noticeable, probably it's not.

[kiril-matev]

Hi @krzysztof-grzybek,

Thanks for looking into this further.

I'm glad to hear you've seen the performance impact of the improvements we've made in our latest versions.

Please see my message just above. Please update the example I've sent to reproduce the issue you're reporting in JavaScript when using the client-side row model. This way we'll be able to confirm whether there is an issue and investigate further.

Looking forward to your response.

[Harpush]

@kiril-matev Hey I ran your example on the same machine - here are the results of first render using the button:

Full time for the click (built from two phases - tasks):

Full time bottom up time taken:

Same as above just for setRowData:

As you can see at least for me the biggest bottleneck is till ParseHTML and generally template creation. I wonder what the performance will be with the createElement approach.