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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Performance improvements #4910
base: b25.3.0
Are you sure you want to change the base?
Performance improvements #4910
Conversation
Hi, Thanks for submitting this PR, at the moment we don鈥檛 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! |
Thank you Alberto! |
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: 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? |
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. 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. |
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. |
@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. |
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: 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. |
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: 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:
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. |
@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. Performance check bottleneck for the three biggest render parts (tasks): |
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. |
@kiril-matev Sure. Its exactly the same example as you sent with a button added. This is it anyway: https://plnkr.co/edit/o7QERYRVuyjzd3wl. |
@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:
|
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:
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. |
@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. |
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. |
@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 As you can see at least for me the biggest bottleneck is till |
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 callingloadTemplate
to create the DOM manually using methods likedocument.createElement
,element.setAttribute
etc. we experienced 5 to 10 times faster execution ofparams.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.