Fix Grid initial render performance #10579

tsuoanttila · 2018-01-30T12:48:16Z

This patch removes unnecessary scheduled commands
Adding multiple columns is batched in both GridConnector and Grid
Removed unnecessary size calculations

This change is

tsuoanttila · 2018-01-30T14:25:21Z

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

client/src/main/java/com/vaadin/client/widgets/Grid.java, line 6575 at r2 (raw file):

        columnsToAdd.forEach(col -> {
            // Register column with grid
            columns.add(col);

This is not using the correct index. causes issues in some tests.

Comments from Reviewable

focbenz · 2018-01-30T15:17:27Z

Compared with 8.2.1 this patch would already improve the first time rendering duration.

Browser	Vaadin Version	Grid Size (C,HC,R)	Render Time (ms)	Request Time (ms)
CH	8.2.1	(100, 0, 1000)	2720	0
CH	8.2.1	(50, 50, 1000)	1909	0
FF	8.2.1	(100, 0, 1000)	6065	0
FF	8.2.1	(50, 50, 1000)	5029	0
IE	8.2.1	(100, 0, 1000)	10637	0
IE	8.2.1	(50, 50, 1000)	7143	0
-------	--------------	------------------	----------------	-----------------
CH	8.4-SNAPSHOT	(100, 0, 1000)	2535	68
CH	8.4-SNAPSHOT	(50, 50, 1000)	1782	38
FF	8.4-SNAPSHOT	(100, 0, 1000)	4998	26
FF	8.4-SNAPSHOT	(50, 50, 1000)	4421	24
IE	8.4-SNAPSHOT	(100, 0, 1000)	8281	36
IE	8.4-SNAPSHOT	(50, 50, 1000)	5739	34

tsuoanttila · 2018-01-31T15:29:22Z

Had to refactor some parts of the Grid setup to fix all the race condition -style issues. In the meantime I may have gotten it a bit faster still. Can you test again?

focbenz · 2018-01-31T15:38:17Z

Sure can do ... let me fire up Maven.
If I did not make any mistakes using the latest SNAPSHOT with the race condition fixes in Commit: ba62b34 [ba62b34]
there is no more performance gain:

Browser	Vaadin Version	Grid Size (C,HC,R)	Render Time (ms)	Request Time (ms)
CH	8.4-SNAPSHOT-RACE	(100, 0, 1000)	2677	74
CH	8.4-SNAPSHOT-RACE	(50, 50, 1000)	1804	37
FF	8.4-SNAPSHOT-RACE	(100, 0, 1000)	5050	29
FF	8.4-SNAPSHOT-RACE	(50, 50, 1000)	4444	29
IE	8.4-SNAPSHOT-RACE	(100, 0, 1000)	8429	48
IE	8.4-SNAPSHOT-RACE	(50, 50, 1000)	5881	35

Will retry the with a clean local Maven repository tomorrow to make sure.
Update: The measured values from the first attempt are still the best ones o.ob

tsuoanttila · 2018-02-01T14:24:18Z

All the tests are once again passing and some of the bit more fragile parts of the Grid should be a bit "sturdier" now. I'll run some performance tests on my end but would be nice to get comparable results to earlier reports here if it's not too much to ask.

That will tell me if I need to go back and trace out more loose ends from the Grid or is this is a good enough improvement to merge and investigate more a bit later

focbenz · 2018-02-01T14:35:23Z

Will pull the latest changes and rerun the tests posting results.
Update:
Thank you for the thorough work on reducing the initial rendering time.

Here are two runs with the latest commit [0675ba0]:

Browser	Vaadin Version	Grid Size (C,HC,R)	Render Time (ms)	Request Time (ms)
CH	8.4-SNAPSHOT-0675ba0	(100, 0, 1000)	2366	53
CH	8.4-SNAPSHOT-0675ba0	(50, 50, 1000)	1782	45
FF	8.4-SNAPSHOT-0675ba0	(100, 0, 1000)	4685	23
FF	8.4-SNAPSHOT-0675ba0	(50, 50, 1000)	4150	29
IE	8.4-SNAPSHOT-0675ba0	(100, 0, 1000)	7179	38
IE	8.4-SNAPSHOT-0675ba0	(50, 50, 1000)	5065	32
CH	8.4-SNAPSHOT-0675ba0	(100, 0, 1000)	2345	70
CH	8.4-SNAPSHOT-0675ba0	(50, 50, 1000)	1741	28
FF	8.4-SNAPSHOT-0675ba0	(100, 0, 1000)	4911	27
FF	8.4-SNAPSHOT-0675ba0	(50, 50, 1000)	4001	20
IE	8.4-SNAPSHOT-0675ba0	(100, 0, 1000)	7230	30
IE	8.4-SNAPSHOT-0675ba0	(50, 50, 1000)	5211	25

elmot · 2018-02-01T15:59:06Z

Reviewed 1 of 3 files at r1, 1 of 2 files at r8, 2 of 2 files at r9.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java, line 322 at r9 (raw file):

        if (!getState().columnOrder.containsAll(idToColumn.keySet())) {
            updateColumns();
        } else if (stateChangeEvent.hasPropertyChanged("columnOrder")) {

Are you sure in this else? Doesn't it prevent column reordering is some situation?

client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java, line 334 at r9 (raw file):

    }

    // @OnStateChange("columnOrder")

To be removed

client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java, line 348 at r9 (raw file):

     * Updates the grid header section on state change.
     */
    // @OnStateChange("header")

To be removed

client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java, line 468 at r9 (raw file):

     * Updates the grid footer section on state change.
     */
    // @OnStateChange("footer")

To be removed

client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java, line 522 at r9 (raw file):

        idToColumn.put(id, column);

        if (idToColumn.keySet().containsAll(getState().columnOrder)) {

Doesn't it prevent column reordering is some situation?

client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java, line 545 at r9 (raw file):

        // Remove old column
        currentColumns.stream()

2nn column list scan. Definitely, here is a room for optimization. Not a blocker.

client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java, line 565 at r9 (raw file):

                && currentColumns.get(0) instanceof SelectionColumn) {
            // ignore selection column.
            currentColumns = currentColumns.subList(1, currentColumns.size());

Code style issue: Parameter is modified

client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java, line 577 at r9 (raw file):

     *            column to remove
     */
    public void removeColumn(CustomColumn column) {

The method name and the javadoc are misleading.

client/src/main/java/com/vaadin/client/widgets/Grid.java, line 6578 at r9 (raw file):

    private void addColumnsSkipSelectionColumnCheck(
            Collection<Column<?, T>> columnsToAdd, int startIndex) {
        AtomicInteger index = new AtomicInteger(0);

might be simplified a little: new AtomicInteger(startIndex) and remove startIndex below

Comments from Reviewable

tsuoanttila · 2018-02-02T09:10:53Z

Review status: all files reviewed at latest revision, 7 unresolved discussions.

client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java, line 322 at r9 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

Are you sure in this else? Doesn't it prevent column reordering is some situation?

The if block is executed when there are columns in the map that are not in the columnOrder -> columns have been removed.

Columns are removed from the map onUnregister which is "too late" for some functionality.

The updateColumns makes the whole Grid match the columnOrder. The updateColumnOrder is only called when only the order has changed.

client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java, line 522 at r9 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

Doesn't it prevent column reordering is some situation?

updateColumns makes the column order in Grid match the state.

This block is only executed when all columns listed in the column order are available in the column map -> all new columns have been added

Comments from Reviewable

tsuoanttila · 2018-02-05T10:19:03Z

Review status: all files reviewed at latest revision, 6 unresolved discussions.

client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java, line 334 at r9 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

To be removed

Done.

client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java, line 348 at r9 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

To be removed

Done.

client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java, line 468 at r9 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

To be removed

Done.

client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java, line 565 at r9 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

Code style issue: Parameter is modified

Done.

client/src/main/java/com/vaadin/client/connectors/grid/GridConnector.java, line 577 at r9 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

The method name and the javadoc are misleading.

Done.

client/src/main/java/com/vaadin/client/widgets/Grid.java, line 6578 at r9 (raw file):

Previously, elmot (Ilia Motornyi) wrote…

might be simplified a little: new AtomicInteger(startIndex) and remove startIndex below

Done.

Comments from Reviewable

elmot · 2018-02-05T13:12:17Z

Reviewed 3 of 3 files at r10.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

Fixes #10232

tsuoanttila mentioned this pull request Jan 30, 2018

8.1 Grid is much slower than table #10232

Closed

Teemu Suo-Anttila added 6 commits February 1, 2018 12:25

Fix Grid initial render performance

3f2048f

Remove extra logging

3f40b64

Fix indexing when adding columns

d2b1db3

Delay column order setting when needed

c42b1bd

Fix race condition with updating columns

2cf6071

Make state handling always the same order

6f6f6c0

tsuoanttila force-pushed the grid_columns_performance branch from ba62b34 to 6f6f6c0 Compare February 1, 2018 11:35

Fix removing frozen columns

0675ba0

Fixes based on review

fae247f

elmot merged commit d1749cb into master Feb 5, 2018

elmot deleted the grid_columns_performance branch February 5, 2018 13:24

tsuoanttila added a commit that referenced this pull request Feb 5, 2018

Fix Grid initial render performance (#10579)

cc7a2b6

Fixes #10232

elmot pushed a commit that referenced this pull request Feb 6, 2018

Fix Grid initial render performance (#10579)

787a1f1

Fixes #10232

tsuoanttila added this to the 8.3.1 milestone Feb 7, 2018

TatuLund mentioned this pull request Feb 12, 2018

Grid column width is wrongly calculated when reattaching component #10438

Closed

tsuoanttila added a commit that referenced this pull request Feb 13, 2018

Fix Grid initial render performance (#10579)

8f06444

Fixes #10232

tsuoanttila added a commit that referenced this pull request Feb 13, 2018

Fix Grid initial render performance (#10579)

3537bda

Fixes #10232

tsuoanttila added a commit that referenced this pull request Feb 13, 2018

Fix Grid initial render performance (#10579)

5b70f3e

Fixes #10232

focbenz mentioned this pull request Feb 23, 2018

Vaadin 8.3.1 initial grid rendering time in browser too slow in Internet Explorer #10656

Closed

TatuLund mentioned this pull request Apr 15, 2018

Grid recalculateColumnWidths doesn't work when using the Grid with a dataProvider #9996

Closed

ericflock mentioned this pull request Jul 11, 2018

Make the Grid fast again #11043

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Grid initial render performance #10579

Fix Grid initial render performance #10579

tsuoanttila commented Jan 30, 2018 •

edited by Artur-

Loading

tsuoanttila commented Jan 30, 2018

focbenz commented Jan 30, 2018 •

edited

Loading

tsuoanttila commented Jan 31, 2018

focbenz commented Jan 31, 2018 •

edited

Loading

tsuoanttila commented Feb 1, 2018 •

edited

Loading

focbenz commented Feb 1, 2018 •

edited

Loading

elmot commented Feb 1, 2018

tsuoanttila commented Feb 2, 2018

tsuoanttila commented Feb 5, 2018

elmot commented Feb 5, 2018

Fix Grid initial render performance #10579

Fix Grid initial render performance #10579

Conversation

tsuoanttila commented Jan 30, 2018 • edited by Artur- Loading

tsuoanttila commented Jan 30, 2018

focbenz commented Jan 30, 2018 • edited Loading

tsuoanttila commented Jan 31, 2018

focbenz commented Jan 31, 2018 • edited Loading

tsuoanttila commented Feb 1, 2018 • edited Loading

focbenz commented Feb 1, 2018 • edited Loading

elmot commented Feb 1, 2018

tsuoanttila commented Feb 2, 2018

tsuoanttila commented Feb 5, 2018

elmot commented Feb 5, 2018

tsuoanttila commented Jan 30, 2018 •

edited by Artur-

Loading

focbenz commented Jan 30, 2018 •

edited

Loading

focbenz commented Jan 31, 2018 •

edited

Loading

tsuoanttila commented Feb 1, 2018 •

edited

Loading

focbenz commented Feb 1, 2018 •

edited

Loading