Grouping Performance Issue

[Vsinghal339-source]

Describe the bug

Requirement: Approximately 60,000 records are available.

The code took about four to five seconds to finish when we used the grouping feature and any of its functions, such as expand all, collapse all, add grouping, and remove grouping. After conducting preliminary investigation, we discovered that the line below was consuming all of the time during the refresh function in the slickDataView.ts file. However, we can't pinpoint the precise underlying reason. Please assist in locating the problematic code.

this.onRowsChanged.notify({ rows: diff, itemCount: this.items.length, dataView: this, calledOnRowCountChanged: (countBefore !== this.rows.length) }, null, this);

Reproduction

this.onRowsChanged.notify({ rows: diff, itemCount: this.items.length, dataView: this, calledOnRowCountChanged: (countBefore !== this.rows.length) }, null, this);

Which Framework are you using?

Vanilla / Plain JS

Environment Info

| Executable          | Version |
| ------------------- | ------- |
| (framework used)    | VERSION |
| Slickgrid-Universal | VERSION |
| TypeScript          | VERSION |
| Browser(s)          | VERSION |
| System OS           | VERSION |

Validations

• Follow our Code of Conduct
• Read the Wikis.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion.
• The provided reproduction is a minimal reproducible example of the bug.

[Vsinghal339-source]

@ghiscoding Please check this

[ghiscoding]

I don't really know but I would say, do a global search for the event name and start commenting code that you find. There's potentially this code below, but it was put in place for the reason described in the code

slickgrid-universal/packages/vanilla-bundle/src/components/slick-vanilla-grid-bundle.ts

Lines 834 to 843 in 8d7683c

	// when filtering data with local dataset, we need to update each row else it will not always show correctly in the UI
	// also don't use "invalidateRows" since it destroys the entire row and as bad user experience when updating a row
	if (gridOptions?.enableFiltering && !gridOptions.enableRowDetailView) {
	this._eventHandler.subscribe(dataView.onRowsChanged, (_e, args) => {
	if (args?.rows && Array.isArray(args.rows)) {
	args.rows.forEach((row: number) => grid.updateRow(row));
	grid.render();
	}
	});
	}

[ghiscoding]

Yeah so after testing a bit with Example 3 which has a button to add 500K items. I added some perf logs and find that we could improve the code has shown below to improve perf a bit but I don't see that much of a difference with 500K items.

this._eventHandler.subscribe(dataView.onRowsChanged, (_e, args) => {
  if (Array.isArray(args?.rows)) {
    console.time('Rows Changed Perf');
    const ranges = this.slickGrid!.getRenderedRange();
    args.rows.forEach((row: number) => {
      if (row >= ranges.top && row <= ranges.bottom) {
        grid.updateRow(row);
      }
    });
    grid.render();
    console.timeEnd('Rows Changed Perf');
  }
});

here's the result of the console time perfs, we can see it's a bit better with condition check but it's not a huge difference. On the Example 3 with 500K, I only see a 1-2sec wait when grouping so it's not what you see on your side but that could vary depending on your data complexity.

# old code
Rows Changed Perf: 31.5830078125 ms
Rows Changed Perf: 30.7841796875 ms
Rows Changed Perf: 30.51513671875 ms

# new code with rendered range check
Rows Changed Perf: 23.989013671875 ms
Rows Changed Perf: 25.4140625 ms
Rows Changed Perf: 28.677978515625 ms

The steps that I used was to go on Example 3 then

1. click on "500K" button to add more items
2. scroll to the middle of the grid
3. use any of the group buttons (but always use the same one for testing)

So even if I don't see much of a difference with the code change, I think we can still push this new code. I'll let you test it out and improve it if you can and wait for your PR. I was planning to do a release tomorrow but can wait until next week if you don't have time to create a PR before tomorrow

If that is not helping then like I said above, just do a global search for onRowsChanged and try commenting other piece of code to find which subscription causes the delay but I really think it's this one here.

EDIT

oh wait, I just discovered that we have a boolean property named calledOnRowCountChanged in the onRowsChanged event arguments and I already have code just above to invalidate all the rows when onRowCountChanged is being called, so there's no reason to invalidate the same rows multiple times (currently by onRowCountChanged (line 821) and onRowsChanged (line 837) subscriptions).

slickgrid-universal/packages/vanilla-bundle/src/components/slick-vanilla-grid-bundle.ts

Lines 820 to 824 in 8d7683c

	// When data changes in the DataView, we need to refresh the metrics and/or display a warning if the dataset is empty
	this._eventHandler.subscribe(dataView.onRowCountChanged, () => {
	grid.invalidate();
	this.handleOnItemCountChanged(this.dataView?.getFilteredItemCount() || 0, this.dataView?.getItemCount() ?? 0);
	});

So with that in mind, we can add a condition to only call the code I mentioned earlier but only when calledOnRowCountChanged is false (this will only happen when you edit a cell, which is why that piece of code was added anyway). Basically, calledOnRowCountChanged is true when rendering, scrolling or grouping (but false when editing a cell) and with that new condition the code invalidate of each row is completely skipped (unless you're editing a cell) and that should improve perf a lot.

this._eventHandler.subscribe(dataView.onRowsChanged, (_e, { calledOnRowCountChanged, rows }) => {
  // filtering data with local dataset will not always show correctly unless we call this updateRow/render
  // also don't use "invalidateRows" since it destroys the entire row and as bad user experience when updating a row
  // see commit: https://github.com/ghiscoding/aurelia-slickgrid/commit/8c503a4d45fba11cbd8d8cc467fae8d177cc4f60
  if (!calledOnRowCountChanged && Array.isArray(rows)) {
    const ranges = grid.getRenderedRange();
    rows
      .filter(row => row >= ranges.top && row <= ranges.bottom)
      .forEach((row: number) => grid.updateRow(row));
    grid.render();
  }
});

I'll let you test this out and let you create a PR with this code if you think it helps (just make sure to remove the console time, it's only there for showing perf times). You might also need to update the unit tests if there's a failure because of code change

Thanks for investigating all of these perfs, it's really beneficial to all of us 😃

[ghiscoding]

So I went ahead and opened a PR #1680, I believe that will fix the slowness you detected and even if doesn't fix your issue, it's still a PR that will improve performances

[Vsinghal339-source]

Yes this PR seems good for some performances