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

GroupBy row select emit event once, perf #10493

Merged
merged 3 commits into from
Nov 15, 2021

Conversation

damyanpetev
Copy link
Member

@damyanpetev damyanpetev commented Nov 12, 2021

Closes #9250

Related to changes in PR #10088; see after screenshot - the bulk of the remaining slow down was caused by the Group selector click handler spamming single selects with event emits for each. Per this #9405 (comment), with 30k-ish records in the group selection took a good 20s+ due to that.

Refactored two methods that do essentially the same as the single ones but in bulk and now the selection emits once but is also now decently quick. For comparison w/ the previous PR scenario:
image

Further, with 30k rows the click handler is in the 115ms range and acceptable 300ms for 100k rows (local dev run w/ profiler running), so it scales more or less linearly. Also being finally profile at higher count, the previously fixed selectedRowsInTheGroup became the slowest link again, so further improved as well.

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

@@ -463,11 +462,44 @@ export class IgxGridSelectionService {
}
const newSelection = this.getSelectedRows().filter(r => r !== rowID);
if (this.rowSelection.size && this.rowSelection.has(rowID)) {
this.selectedRowsChange.next();
Copy link
Member Author

@damyanpetev damyanpetev Nov 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed multiple instances of sending new values to the subject before calling emitRowSelectionEvent. The latter will eventually reach and call selectedRowsChange.next() as well, but after the cancel check..

BEHAVIOR CHANGE: row selection even is now emitted only once
for the entire group selected as intended.
`added` and `removed` arrays in args contain all affected rows

Closes #9250
@damyanpetev damyanpetev force-pushed the dpetev/groupby-row-select-perf-event branch from e70feeb to 3fba870 Compare November 12, 2021 15:31
@damyanpetev damyanpetev marked this pull request as ready for review November 12, 2021 15:31
@damyanpetev damyanpetev added the ❌ status: awaiting-test PRs awaiting manual verification label Nov 12, 2021
@mmart1n mmart1n added 💥 status: in-test PRs currently being tested and removed ❌ status: awaiting-test PRs awaiting manual verification labels Nov 15, 2021
@mmart1n
Copy link
Contributor

mmart1n commented Nov 15, 2021

image

Scenario 19 has failed. When editing a selected row so that it goes to another group the row appears as not selected and the group-by row checkboxes are not handled correctly. If this is the only one selected row in the grid the header checkbox selector remains in indeterminate state.

@mmart1n
Copy link
Contributor

mmart1n commented Nov 15, 2021

image

Scenario 19 has failed. When editing a selected row so that it goes to another group the row appears as not selected and the group-by row checkboxes are not handled correctly. If this is the only one selected row in the grid the header checkbox selector remains in indeterminate state.

This issue is not introduced by this PR so it should be logged separately.

@mmart1n mmart1n added ✅ status: verified Applies to PRs that have passed manual verification and removed 💥 status: in-test PRs currently being tested labels Nov 15, 2021
@rkaraivanov rkaraivanov merged commit 461f2e9 into master Nov 15, 2021
@rkaraivanov rkaraivanov deleted the dpetev/groupby-row-select-perf-event branch November 15, 2021 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GroupBy selection event emitted multiple times
3 participants