Make the Grid fast again #11043
Make the Grid fast again #11043
Conversation
|
|
|
@tsuoanttila The most tests failed due to a NoSuchElementException, but I cannot reproduce it on my local machine. The elements with the selectors to find are all there. Could it be a race condition? Furthermore some tests fail, although I changed nothing on the testet component itself, e.g. WebBrowserTimeZoneTest or ComboBoxItemIconTest. Are the tests currently working? Thanks in advance |
|
We had some recent changes in our testing environment and have not had time to fix the issues for Vaadin 7. These problems are being worked on. Once we have them fixed, you'll need to rebase your change on top of latest |
|
Validation build has now been fixed, I updated the PR. |
|
@tsuoanttila Thank you for your help :) |
|
Should be ported to compatibility if merged |
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 6 unresolved discussions (waiting on @ericflock)
client/src/main/java/com/vaadin/client/connectors/GridConnector.java, line 175 at r1 (raw file):
@SuppressWarnings("unchecked") public CustomGridColumn(GridColumnState state) {
API breaking change. Is it possible to keep old constructor parameters and then get the state via AbstractRendererConnector.getState()
client/src/main/java/com/vaadin/client/widgets/Grid.java, line 6485 at r1 (raw file):
private <C extends Column<?, T>> void addColumnsSkipSelectionColumnCheck(Collection<C> columnCollection, int index) { // final Map<Integer, Double> columnWidths = new HashMap<>();
Dead code to be removed
client/src/main/java/com/vaadin/client/widgets/Grid.java, line 6487 at r1 (raw file):
// final Map<Integer, Double> columnWidths = new HashMap<>(); int visibleNewColumns = 0; // int currentColumnIndex = index;
Dead code to be removed
client/src/main/java/com/vaadin/client/widgets/Grid.java, line 6500 at r1 (raw file):
if (!column.isHidden()) { visibleNewColumns++; // if (column.getWidth() > 0) {
Dead code to be removed
client/src/main/java/com/vaadin/client/widgets/Grid.java, line 6504 at r1 (raw file):
// } } // currentColumnIndex++;
Dead code to be removed
client/src/main/java/com/vaadin/client/widgets/Grid.java, line 6509 at r1 (raw file):
final ColumnConfiguration columnConfiguration = this.escalator.getColumnConfiguration(); columnConfiguration.insertColumns(index, visibleNewColumns); // columnConfiguration.setColumnWidths(columnWidths);
Dead code to be removed
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 7 unresolved discussions (waiting on @ericflock)
a discussion (no related file):
The change should be verified using all the visual tests
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 7 unresolved discussions (waiting on @elmot and @ericflock)
a discussion (no related file):
Previously, elmot (Ilia Motornyi) wrote…
The change should be verified using all the visual tests
The final change passes the validation build including all framework UI-Tests. Are there any other visual tests I have to run this change on?
client/src/main/java/com/vaadin/client/connectors/GridConnector.java, line 175 at r1 (raw file):
Previously, elmot (Ilia Motornyi) wrote…
API breaking change. Is it possible to keep old constructor parameters and then get the state via
AbstractRendererConnector.getState()
I will provide a new commit, for this.
client/src/main/java/com/vaadin/client/widgets/Grid.java, line 6485 at r1 (raw file):
Previously, elmot (Ilia Motornyi) wrote…
Dead code to be removed
Done.
client/src/main/java/com/vaadin/client/widgets/Grid.java, line 6487 at r1 (raw file):
Previously, elmot (Ilia Motornyi) wrote…
Dead code to be removed
Done.
client/src/main/java/com/vaadin/client/widgets/Grid.java, line 6500 at r1 (raw file):
Previously, elmot (Ilia Motornyi) wrote…
Dead code to be removed
Done.
client/src/main/java/com/vaadin/client/widgets/Grid.java, line 6504 at r1 (raw file):
Previously, elmot (Ilia Motornyi) wrote…
Dead code to be removed
Done.
client/src/main/java/com/vaadin/client/widgets/Grid.java, line 6509 at r1 (raw file):
Previously, elmot (Ilia Motornyi) wrote…
Dead code to be removed
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 7 unresolved discussions (waiting on @elmot and @ericflock)
client/src/main/java/com/vaadin/client/connectors/GridConnector.java, line 175 at r1 (raw file):
Previously, ericflock wrote…
I will provide a new commit, for this.
New commit isn't needed. The change does not break the api.
The CustomGridColumn is an private inner class of the GridConnector. So only the GridConnector can call this constructor or extend the class.
Your suggestion to get the state via AbstractRendererConnector.getState() doesn't work here, because the relevant state is of type GridColumnState.
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1, 1 of 2 files at r9, 1 of 1 files at r10.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @elmot)
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1, 1 of 2 files at r9, 1 of 1 files at r10.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @elmot)
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1, 1 of 2 files at r9, 1 of 1 files at r10.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @elmot)
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 7 unresolved discussions (waiting on @elmot)
client/src/main/java/com/vaadin/client/connectors/GridConnector.java, line 175 at r1 (raw file):
Previously, ericflock wrote…
New commit isn't needed. The change does not break the api.
The CustomGridColumn is an private inner class of the GridConnector. So only the GridConnector can call this constructor or extend the class.Your suggestion to get the state via AbstractRendererConnector.getState() doesn't work here, because the relevant state is of type GridColumnState.
The public constructor parameters are changed => breaking change in API. Probably we can survive with this, since it is on client side
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1, 1 of 2 files at r9, 1 of 1 files at r10.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @elmot)
a discussion (no related file):
Previously, ericflock wrote…
The final change passes the validation build including all framework UI-Tests. Are there any other visual tests I have to run this change on?
There is a "full" build process, with a huge pile of tests. It should be run with this change, but we need to stabilize the mainstream build first. I am blocking the PR until it's done and we can move forward.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ericflock)
client/src/main/java/com/vaadin/client/connectors/GridConnector.java, line 175 at r1 (raw file):
Previously, elmot (Ilia Motornyi) wrote…
The public constructor parameters are changed => breaking change in API. Probably we can survive with this, since it is on client side
Ok... very strict, but I will revert and change it.
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @elmot)
a discussion (no related file):
Previously, elmot (Ilia Motornyi) wrote…
There is a "full" build process, with a huge pile of tests. It should be run with this change, but we need to stabilize the mainstream build first. I am blocking the PR until it's done and we can move forward.
Ok. I do not exactly know what you mean, because this is my first pull request to vaadin and I do not know how the process works. Please let me know, if or what I need to do.
|
@ericflock, thanks for your patience, the ball is on our side ATM. People outside the company can not run those validations because of security risks. |
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @elmot)
|
Hi @ericflock , we are not going to merge this pull request as is. But it is certainly trying to tackle real issues and work has been already beneficial and motivated us to study this further. However, it would be easier to process this in smaller pieces. We merged this other patch recently, and it is doing some performance improvement that is proposed also in your patch in less evasive way #11199 So thus rebasing and refactoring is needed. There is a way to improve the performance in your patch avoiding unnecessary width calculation of the multiselection checkbox column in your patch. This is something we are still interested in. So if you would be interested to refactor that to separate pull request, that would be most certainly welcome. |
cause it leads to a wrong focused cell, and also no backend data will be requested initially.
Make the Grid fast again.
As described in several issues, the Grid render performance is not as good as expected, especially if it's configured with many columns.
This change solves these issues by avoiding loops with multiple calls to the same functions for every column.
This was also introduced in parts by tsuoanttila with #10579 for Vaadin 8. This change solves these issues in Vaadin 7.7
Additionally removed the column-width calculation in escaltor on paintInsertRows, because the calculation will be anyway scheduled.
Because this is a performance issue, no test classes are provided, instead I provide some benchmark data to show the improvement of this change.
This benchmark was measured with Chrome 67.
grid-performance.pdf
As you can see the performance increases significantly.
This is the first change to increase the performance of the Grid in Vaadin 7.7. A second change, which optimizes the automatic column width calculation, will follow in case this pull request will be successfully merged.
This change is