HideColumnByIds performance

[ttzn]

Clear and concise description of the problem

Hello,

I've been using slickgrid-react to power a live-updating grid containing thousands of rows, with quite some success. This grid has external controls to toggle visibility of groups of columns, some of which contain more than a dozen columns with various stylings. The problem is that hiding 10 columns at a time takes several seconds during which the UI is frozen, and this is due to the fact that hideColumnByIds calls hideColumnById for each provided column, causing unneeded re-renders.

This problem is compounded by the fact that there doesn't seem to be a straightforward way of showing back hidden columns programmatically (unless I missed something in the documentation), so I'm currently resorting to resetting the whole grid then hiding fields which are still toggled out; needless to say it takes way longer than it needs to.

Suggested solution

hideColumnByIds should be reimplemented to only call setColumns after building the final array of visible columns; I'm willing to open a PR for this. A solution to my secondary problem would be to add a showColumnsById method to the GridService, if you're OK with both changes in a single PR (otherwise I can open a separate issue).

Alternative

I'm not sure if setColumns or changing sharedService.visibleColumns trigger any logic or events across the framework that someone might rely on; if you want to be very conservative this new behaviour could be made opt-in through an additional configuration property.

Additional context

Thanks for all the hard work!

Validations

• Follow our Code of Conduct
• Read the Wikis.
• Check that there isn't already an issue that request the same feature to avoid creating a duplicate.

[ghiscoding]

hmm I'm never using this function for more than 1 or 2 columns and never with a dozen of columns and so I've never noticed the perf impact. It might be caused by the autosizeColumns() being called for each column, you could try commenting out the code to see if it helps and if it does help then maybe add another check/option to disable when running on an array similar to what I've done with options.triggerEvent It's actually the setColumns() that is being called each time, see post below.

slickgrid-universal/packages/common/src/services/grid.service.ts

Lines 219 to 227 in 4c4fb79

	// do we want to auto-resize the columns in the grid after hidding some? most often yes
	if (options?.autoResizeColumns) {
	this._grid.autosizeColumns();
	}
	// do we want to trigger an event after hidding
	if (options?.triggerEvent) {
	this.pubSubService.publish('onHeaderMenuHideColumns', { columns: visibleColumns });
	}

Ahhh, it looks like I've already thought of adding the option in the plural function but I forgot to also add it to the singular function (it should be disabled in the singular when looping, same as what I've done for triggerEvent) which should definitely be implemented (below is the option used in the plural function)

slickgrid-universal/packages/common/src/services/grid.service.ts

Lines 246 to 249 in 4c4fb79

	// do we want to auto-resize the columns in the grid after hidding some? most often yes
	if (options?.autoResizeColumns) {
	this._grid.autosizeColumns();
	}

Technically speaking SlickGrid always worked with the concept of any columns missing when calling the grid.setColumns() will be considered hidden. It's always been this way and SlickGrid was created with that in mind. However, another maintainer of SlickGrid (6pac/Ben) added a hidden column property but I have not used it yet and it works very differently (it will remain part of the column defs but will not create the associated column div). This hidden is to "use at your own risks" because it's not fully tested and it's also not used at all especially for ColumnPicker/GridMenu/GridState/... and so it might have weird behaviors. I'm considering trying and possibly switching to the hidden in the next major version but until then, it's probably better not to use it... so we're back to the concept of "if it's not in setColumns(), it's considered hidden"

[ghiscoding]

Actually I also see that setColumns() is also called for each column change, that's bad and most probably the perf issue and that also have to change via an option. So I think that a PR should add a new option in the singular function to disable this one when called by the plural function

• options.applySetColumns

This option should default to true in the singular function (same as options.triggerEvent) but assigned as false when called by the plural function. However, we can't skip them completely, we would still need to call them but only within the plural function. Similar to this below and instead of 2 options, we would have 3 options

slickgrid-universal/packages/common/src/services/grid.service.ts

Lines 242 to 253 in 4c4fb79

	for (const columnId of columnIds) {
	// hide each column by its id but wait after the for loop to auto resize columns in the grid
	this.hideColumnById(columnId, { ...options, triggerEvent: false, autoResizeColumns: false });
	}
	// do we want to auto-resize the columns in the grid after hidding some? most often yes
	if (options?.autoResizeColumns) {
	this._grid.autosizeColumns();
	}
	// do we want to trigger an event after hidding
	if (options?.triggerEvent) {
	this.pubSubService.publish('onHeaderMenuHideColumns', { columns: this.sharedService.visibleColumns });
	}

@ttzn Would you like to contribute a fix?

[ttzn]

Thanks for looking into this.

Actually I also see that setColumns() is also called for each column change, that's bad and most probably the perf issue

Not just probably, but positively. This can easily be seen in any browser profiler, that's how I found the issue in the first place.

Would you like to contribute a fix?

I will open a PR with your suggested fix shortly.

What about adding a showColumnsById method? I understand your point that Slickgrid under the hood doesn't really have a concept of visibility, but hideColumnByIds is useful and straightforward, might as well have its counterpart, WDYT?

[ghiscoding]

What about adding a showColumnsById method? I understand your point that Slickgrid under the hood doesn't really have a concept of visibility, but hideColumnByIds is useful and straightforward, might as well have its counterpart, WDYT?

I'm not sure it's really needed since you can simply call grid.setColumns() directly with the column ids and you're pretty much done. So I don't see much benefit in adding the show function. Everything in the Grid Service, where you found the hide function, can be done by yourself and I only added them to avoid duplicating certain implementation over and over.

[ghiscoding]

Just to be clear, I wouldn't mind if you add a showColumnsById() just to align the functions, especially if you call them both in the same page. Though its implementation is rather small, 2 lines of code (or maybe 4 with if check of this._grid) 😆

showColumnsById(columnIds: Column[]) {
  this._grid.setColumns(columnIds);
  this.sharedService.visibleColumns = columnIds; // keep ref for ColumnPicker/GridMenu
}

though you'll need to add unit tests for this because the entire project has unit tests 😉

and to avoid duplicating code, I guess you could refactor the code to call this new function have less duplicated code and automatically covers test coverage (I would however still prefer a single unit test for it). The lines below have to be refactored anyway with the other suggestion with a new option, so using this new show function inside the new if condition might be a good refactoring to do.

slickgrid-universal/packages/common/src/services/grid.service.ts

Lines 204 to 207 in 4c4fb79

	if (colIndexFound >= 0) {
	const visibleColumns = arrayRemoveItemByIndex<Column>(currentColumns, colIndexFound);
	this.sharedService.visibleColumns = visibleColumns;
	this._grid.setColumns(visibleColumns);

[ttzn]

I see you went ahead with the fix yourself, thanks for taking care of this! When can we expect a release?

[ghiscoding]

fixes are released on all repos, cheers. If you like the lib you can also ⭐ 😉
Side note, I actually had to do 2 PRs to fix this because after retrying, I saw it wasn't working correctly, it took a bit more work
PR #1736 and #1738