wt icon indicating copy to clipboard operation
wt copied to clipboard

Improved performance of adding rows and columns to the WTable

Open jakub-w opened this issue 5 years ago • 2 comments

The issue was the calling of WTableRow::rowNum() for every added cell, which iterated over the whole table.

The methods WTableRow::createCell(), WTableRow::insertColumn() and WTableRow::expand() got the new row argument defaulting to -1 that WTable can use when creating new elements with WTable::insertRow() and WTable::insertColumn(). These methods are called by WTable::expand() so the performance improvement affects every operation that expands the table.

The performance of populating a new table row by row is increased with this commit from O(n^2+n) to O(n).

jakub-w avatar May 09 '19 14:05 jakub-w

I'm not sure about this approach of smuggling the row number into the insertColumn() call, thereby changing the signature virtual method WTableRow::createCell().

I do wonder why that method is protected virtual yet not documented. Maybe it does not need to be. Also, it appears that WTable::createCell() does not use the row and column number by default, so it is a bit unfortunate that rowNum() gets called and then eventually the result is unused.

I think it is best that the WTableRow contains its row number (meaning that any insertions or deletions of rows require an update of the row numbers), and perhaps similarly, WTableColumn contains its column number.

Regards, Roel

emweb avatar May 21 '19 12:05 emweb

I somehow missed WTable::createCell() was virtual. Probably because I wanted to fix it quickly and move on. This is the reason for the delay in my activity too, sorry for that. I modified the pull request as suggested.

The two tests in 989c2ad are disabled because of a bug they uncovered. I didn't want to fix it in this pull request but didn't want to remove the tests. When moving from the middle of the vector to its end an extra item is added after the inserted one, so the tests show that the vector is longer than it should be.

jakub-w avatar Oct 05 '19 17:10 jakub-w