Improved performance of adding rows and columns to the WTable #154

jakub-w · 2019-05-09T14:04:53Z

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).

The issue was the calling of WTableRow::rowNum() for every added cell, which iterated over the whole table. The methods WTableRow::createCell() and WTableRow::insertColumn() 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).

emweb · 2019-05-21T12:36:29Z

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

This reverts commit 316f6b1.

Two of them (moveRow2 and moveColumn2) do not pass because of a bug in the WTable's code, which is not adressed in this commit. That's why these ones are disabled. The tests are not that comprehensive and were created to test the correctnes of code in future commits adressing WTable's performance.

- Added new field to WTableRow - rowNumber_ - to track its position in the table_. - Added WTable::updateRowNumbers() function to update row numbers for rows that need updating. - Modified WTable::insertRow(), WTable::removeRow() and WTable::moveRow() to update row numbers for the rows affected by these operations. - Modified WTableCell::rowNum() to use WTableCell::rowNumber_ if present. Adding rows at the end of the array is now much, much faster. It doesn't need to iterate over every cell anymore. Modified methods from WTable are a little slower than before when removing, inserting or moving rows from the middle of the array. This is because of added overhead of having to update row numbers but this slowdown is negligible.

jakub-w · 2019-10-05T17:52:19Z

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.

emweb

I've got some more comments before this could actually be merged into Wt.

emweb · 2022-12-01T11:25:45Z

src/Wt/WTableRow.h

@@ -148,6 +148,7 @@ class WT_API WTableRow : public WObject
  std::string id_;
  WT_USTRING styleClass_;
  bool hidden_, hiddenChanged_, wasHidden_;
+  size_t rowNumber_;


This should probably be an int for consistency.

emweb · 2022-12-01T11:27:01Z

test/widgets/WTableTest.C

@@ -0,0 +1,269 @@
+/*
+ * Copyright (C) 2010 Emweb bvba, Kessel-Lo, Belgium.


Suggested change

* Copyright (C) 2010 Emweb bvba, Kessel-Lo, Belgium.

* Copyright (C) 2022 Emweb bv, Herent, Belgium.

emweb · 2022-12-01T11:29:16Z

test/widgets/WTableTest.C

+Wt::Test::WTestEnvironment environment;
+Wt::WApplication testApp(environment);


Don't create a WTestEnvironment and WApplication. Every test should create its own, so that tests are nicely isolated.

emweb · 2022-12-01T11:30:00Z

test/widgets/WTableTest.C

+Wt::Test::WTestEnvironment environment;
+Wt::WApplication testApp(environment);
+
+void CheckRowNumbers(WTable* table)


Suggested change

void CheckRowNumbers(WTable* table)

void checkRowNumbers(WTable* table)

Wt uses lowerCamelCase for functions.

emweb · 2022-12-01T11:30:09Z

test/widgets/WTableTest.C

+    BOOST_REQUIRE_EQUAL(table->rowAt(i)->rowNum(), i);
+  }
+}
+void CheckColumnNumbers(WTable* table)


Suggested change

void CheckColumnNumbers(WTable* table)

void checkColumnNumbers(WTable* table)

emweb · 2022-12-01T11:35:29Z

src/Wt/WTable.h

@@ -197,6 +197,7 @@ class WT_API WTable : public WInteractWidget
  void expand(int row, int column, int rowSpan, int columnSpan);
  WTableCell *itemAt(int row, int column);
  void setItemAt(int row, int column, std::unique_ptr<WTableCell> cell);
+  void updateRowNumbers(int begin, int end);


firstRow and lastRow is probably more clear than begin and end. It's also best that you throw in some documentation that clarifies that the lastRow is inclusive.

emweb · 2022-12-01T11:37:15Z

test/widgets/WTableTest.C

+
+BOOST_AUTO_TEST_CASE( elementAt1 )
+{
+  auto table = cpp14::make_unique<WTable>();


cpp14::make_unique was deprecated: use std::unique now instead.

emweb · 2022-12-01T11:38:31Z

src/Wt/WTable.C

@@ -451,6 +458,16 @@ void WTable::moveColumn(int from, int to)
  repaint(RepaintFlag::SizeAffected);
 }

+void WTable::updateRowNumbers(int begin, int end) {
+  if (begin < 0 || begin >= rowCount() || end < 0 || end >= rowCount()) {
+    return;


If this ever happens that would be a programming error that's internal to Wt, right?

This should then either cause an alarming error message or should be omitted altogether.

emweb · 2022-12-01T11:39:04Z

src/Wt/WTable.C

+    return;
+  }
+
+  for (size_t i = begin; i <= end; ++i) {


This loop uses size_t, while begin and end are ints?

emweb · 2022-12-01T11:42:22Z

src/Wt/WTable.C

  rows_.insert(rows_.begin() + row, std::move(tableRow));
+  updateRowNumbers(std::min(old_end, row), rowCount() - 1);


In what situation would this not suffice?

Suggested change

updateRowNumbers(std::min(old_end, row), rowCount() - 1);

updateRowNumbers(row, rowCount() - 1);

jakub-w added 3 commits October 5, 2019 09:32

Revert "Improved performance of adding rows and columns to the WTable."

72bb202

This reverts commit 316f6b1.

emweb suggested changes Dec 1, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved performance of adding rows and columns to the WTable #154

Improved performance of adding rows and columns to the WTable #154

jakub-w commented May 9, 2019

emweb commented May 21, 2019

jakub-w commented Oct 5, 2019

emweb left a comment

emweb Dec 1, 2022

emweb Dec 1, 2022

emweb Dec 1, 2022

emweb Dec 1, 2022

emweb Dec 1, 2022

emweb Dec 1, 2022

emweb Dec 1, 2022

emweb Dec 1, 2022

emweb Dec 1, 2022

emweb Dec 1, 2022

		@@ -0,0 +1,269 @@
		/*
		* Copyright (C) 2010 Emweb bvba, Kessel-Lo, Belgium.

	* Copyright (C) 2010 Emweb bvba, Kessel-Lo, Belgium.
	* Copyright (C) 2022 Emweb bv, Herent, Belgium.

		Wt::Test::WTestEnvironment environment;
		Wt::WApplication testApp(environment);

	void CheckRowNumbers(WTable* table)
	void checkRowNumbers(WTable* table)

	void CheckColumnNumbers(WTable* table)
	void checkColumnNumbers(WTable* table)

		rows_.insert(rows_.begin() + row, std::move(tableRow));
		updateRowNumbers(std::min(old_end, row), rowCount() - 1);

	updateRowNumbers(std::min(old_end, row), rowCount() - 1);
	updateRowNumbers(row, rowCount() - 1);

Improved performance of adding rows and columns to the WTable #154

Are you sure you want to change the base?

Improved performance of adding rows and columns to the WTable #154

Conversation

jakub-w commented May 9, 2019

emweb commented May 21, 2019

jakub-w commented Oct 5, 2019

emweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment