-
Notifications
You must be signed in to change notification settings - Fork 157
Perf. improvements with activation and cell merge. #16251
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
base: 20.1.x
Are you sure you want to change the base?
Conversation
@dkamburov First one, I was not able to reproduce: Are you sure you have the latest changes in the branch or are there any additional steps needed to reproduce? Second one is a sample issue. Because of the way the data is added, the same object refs are used in the grid sample:
Hence, duplicate keys. I fixed it in the sample. |
@MayaKirova appologize on the non reproducible sample: I do had some changes on that sample(didn't noticed them at first as they were from my previous testing on the cell merging):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional, issues I found:
- If you click on any of the headers in the dev sample an error is thrown.

- Go to the performance project and enable the merging on all columns. Run the performance samples using
npm run start:performance
and go to the grid with 1 million records. Sort thePosition
column. Click on the first cell of theRegistered
column. An error is thrown with maximum callstack exceeded.

|
||
} | ||
result.splice(index, res.length, ...res); | ||
result = result.slice(0, index).concat(res, result.slice(index + res.length)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use a regular for-of loop here and instead of creating 3 new array coppies we can mutate the result (which is cloned beforehand) in place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean going through all items in res
and replacing each one in the original array with the unmerged version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, we could. But it would look a bit ugly and also not sure how much more performant it would be considering that in the particular scenario:
- Go to the performance project and enable the merging on all columns. Run the performance samples using
npm run start:performance
and go to the grid with 1 million records. Sort thePosition
column. Click on the first cell of theRegistered
column. An error is thrown with maximum callstack exceeded.![]()
It would need to loop through 300k+ records and replace each one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that
for(let i =0;i<res.length;++i) {
result[index+i] = res[i];
}
would be faster because there is no array copying but changes the result in place. However, seems like the slice
and concat
version is perhaps more optimized by the js engine. I have tested this and, indeed, your current version is faster.
mouse wheel when there is cell merging is becoming choppy, it was faster before the changes and there is a difference when compared to a grid without cell merging |
@dkamburov I can confirm that none of the merge related pipes are actually triggered on mouse wheel, so it doesn't make sense for the change to effect it. Are you sure it's not simply because of the large amount of data in the grid from the sample? It's choppy for me even without merging. |
Closes #16146
Split pipes in two, where one does the merging and the other tracks active rows and unmerges within related merge sequences.
Additional information (check all that apply):
Checklist:
feature/README.MD
updates for the feature docsREADME.MD
CHANGELOG.MD
updates for newly added functionalityng update
migrations for the breaking changes (migrations guidelines)