Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor!: run data generators only for visible columns #6798

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+406 −62
Open

refactor!: run data generators only for visible columns #6798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2fb7739
refactor: schedule renderers to only run for visible columns
ugur-vaadin Nov 7, 2024
361ccde
fix: remove unnecessary line
ugur-vaadin Nov 7, 2024
4128359
fix: send items only once to the client
ugur-vaadin Nov 8, 2024
9959a86
refactor: patch data generator instead of scheduling renderer
ugur-vaadin Nov 11, 2024
c359c25
chore: remove extra empty lines
ugur-vaadin Nov 11, 2024
982bb82
fix: override all methods when patching data generator
ugur-vaadin Nov 12, 2024
337eab2
test: revert unit test asserions
ugur-vaadin Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 40 additions & 57 deletions ...-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the utility of this change: This sort of moves the performance issue to a different use-case, which is showing and hiding columns. In that case it now needs to reload all data whenever a single column is made visible. Previously it would load everything up front, but then doesn't have to reload everything if you show a single column.

What is everyone's thought on this?

Copy link
Member

@tomivirkki tomivirkki Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, hiding and unhiding columns shouldn't need to result in a data provider call, but only the data generators should need to be rerun for the already loaded items.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure that whoever created this issue wouldn't expect additional data provider calls as part of this change.

Since there's a trade-off, maybe it could be turned into an option. Though it's a bit challenging to communicate what this even does.

Copy link
Member

@tomivirkki tomivirkki Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This nasty prototype method in Grid does the job without additional data provider calls, but it requires reflection. I hope there's a better way (maybe storing the value in Grid.setRequestedRange):

private void regenerateData() {
        try {
        var communicator = getDataCommunicator();
        var requestedRangeField = communicator.getClass()
                .getDeclaredField("requestedRange");
        requestedRangeField.setAccessible(true);
        var requestedRange = (Range) requestedRangeField
                .get(communicator);

        for (int i = requestedRange.getStart(); i < requestedRange.getEnd(); i++) {
            var item = communicator.getItem(i);
            if (item != null) {
                getDataCommunicator().refresh(item);
            }
        }
    } catch (Exception e) {
        // ignore
    }
}

Copy link
Contributor

@vursen vursen Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be related: #6713

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When adding a column, it sends two full updates to the client. One based on the old column config, and then another one with the new config. Also calls the data provider twice.

Bildschirmfoto 2024-11-08 um 10 34 10

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a fix and the test for it to handle this case.

Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.util.Set;
import java.util.function.BiFunction;
import java.util.function.BinaryOperator;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;
Expand Down Expand Up @@ -430,12 +431,6 @@ public static void setDefaultMultiSortPriority(MultiSortPriority priority) {
/**
* Server-side component for the {@code <vaadin-grid-column>} element.
*
* <p>
* Every added column sends data to the client side regardless of its
* visibility state. Don't add a new column at all or use
* {@link Grid#removeColumn(Column)} to avoid sending extra data.
* </p>
*
* @param <T>
* type of the underlying grid this column is compatible with
*/
Expand Down Expand Up @@ -500,8 +495,45 @@ public Column(Grid<T> grid, String columnId, Renderer<T> renderer) {
.getDataGenerator();

if (dataGenerator.isPresent()) {
var generator = dataGenerator.get();

// Use an anonymous class instead of Lambda to prevent potential
// deserialization issues when used with Grid
// see https://github.com/vaadin/flow-components/issues/6256
var conditionalDataGenerator = new DataGenerator<T>() {
@Override
public void generateData(T item, JsonObject jsonObject) {
if (Column.this.isVisible()) {
generator.generateData(item, jsonObject);
}
}

@Override
public void destroyData(T item) {
generator.destroyData(item);
}

@Override
public void destroyAllData() {
generator.destroyAllData();
}

@Override
public void refreshData(T item) {
generator.refreshData(item);
}
};
columnDataGeneratorRegistration = grid
.addDataGenerator(dataGenerator.get());
.addDataGenerator(conditionalDataGenerator);
}
}

@Override
public void setVisible(boolean visible) {
boolean resetDataCommunicator = visible && !isVisible();
super.setVisible(visible);
if (resetDataCommunicator) {
getGrid().getDataCommunicator().reset();
}
}

Expand Down Expand Up @@ -770,7 +802,7 @@ public Column<T> setComparator(Comparator<T> comparator) {
* the value provider used to extract the {@link Comparable}
* sort key
* @return this column
* @see Comparator#comparing(java.util.function.Function)
* @see Comparator#comparing(Function)
*/
public <V extends Comparable<? super V>> Column<T> setComparator(
ValueProvider<T, V> keyExtractor) {
Expand Down Expand Up @@ -1851,11 +1883,6 @@ protected GridArrayUpdater createDefaultArrayUpdater(
* see {@link #addColumn(Renderer)}.
* </p>
* <p>
* Every added column sends data to the client side regardless of its
* visibility state. Don't add a new column at all or use
* {@link Grid#removeColumn(Column)} to avoid sending extra data.
* </p>
* <p>
* <em>NOTE:</em> This method is a shorthand for
* {@link #addColumn(ValueProvider, BiFunction)}
* </p>
Expand Down Expand Up @@ -1883,11 +1910,6 @@ public Column<T> addColumn(ValueProvider<T, ?> valueProvider) {
* {@link #addComponentColumn(ValueProvider)}. For using build-in renderers,
* see {@link #addColumn(Renderer)}.
* </p>
* <p>
* Every added column sends data to the client side regardless of its
* visibility state. Don't add a new column at all or use
* {@link Grid#removeColumn(Column)} to avoid sending extra data.
* </p>
*
* @param valueProvider
* the value provider
Expand Down Expand Up @@ -1946,11 +1968,6 @@ private String formatValueToSendToTheClient(Object value) {
* <em>NOTE:</em> Using {@link ComponentRenderer} is not as efficient as the
* built in renderers or using {@link LitRenderer}.
* </p>
* <p>
* Every added column sends data to the client side regardless of its
* visibility state. Don't add a new column at all or use
* {@link Grid#removeColumn(Column)} to avoid sending extra data.
* </p>
*
* @param componentProvider
* a value provider that will return a component for the given
Expand All @@ -1974,12 +1991,6 @@ public <V extends Component> Column<T> addComponentColumn(
* automatically configured using the return type of the given
* {@link ValueProvider}.
*
* <p>
* Every added column sends data to the client side regardless of its
* visibility state. Don't add a new column at all or use
* {@link Grid#removeColumn(Column)} to avoid sending extra data.
* </p>
*
* @see Column#setComparator(ValueProvider)
* @see Column#setSortProperty(String...)
* @see #removeColumn(Column)
Expand Down Expand Up @@ -2014,11 +2025,6 @@ public <V extends Comparable<? super V>> Column<T> addColumn(
* or using {@link LitRenderer}.
* </p>
* <p>
* Every added column sends data to the client side regardless of its
* visibility state. Don't add a new column at all or use
* {@link Grid#removeColumn(Column)} to avoid sending extra data.
* </p>
* <p>
* <em>NOTE:</em> This method is a shorthand for
* {@link #addColumn(Renderer, BiFunction)}
* </p>
Expand Down Expand Up @@ -2051,11 +2057,6 @@ public Column<T> addColumn(Renderer<T> renderer) {
* {@link ComponentRenderer} is not as efficient as the built in renderers
* or using {@link LitRenderer}.
* </p>
* <p>
* Every added column sends data to the client side regardless of its
* visibility state. Don't add a new column at all or use
* {@link Grid#removeColumn(Column)} to avoid sending extra data.
* </p>
*
* @param renderer
* the renderer used to create the grid cell structure
Expand Down Expand Up @@ -2159,12 +2160,6 @@ protected BiFunction<Renderer<T>, String, Column<T>> getDefaultColumnFactory() {
* from a bean type with {@link #Grid(Class)}.
*
* <p>
* Every added column sends data to the client side regardless of its
* visibility state. Don't add a new column at all or use
* {@link Grid#removeColumn(Column)} to avoid sending extra data.
* </p>
*
* <p>
* <strong>Note:</strong> This method is a shorthand for
* {@link #addColumn(String, BiFunction)}
* </p>
Expand Down Expand Up @@ -2199,12 +2194,6 @@ public Column<T> addColumn(String propertyName) {
* <strong>Note:</strong> This method can only be used for a Grid created
* from a bean type with {@link #Grid(Class)}.
*
* <p>
* Every added column sends data to the client side regardless of its
* visibility state. Don't add a new column at all or use
* {@link Grid#removeColumn(Column)} to avoid sending extra data.
* </p>
*
* @see #addColumn(String)
* @see #removeColumn(Column)
*
Expand Down Expand Up @@ -2276,12 +2265,6 @@ private Object runPropertyValueGetter(PropertyDefinition<T, ?> property,
* <strong>Note:</strong> This method can only be used for a Grid created
* from a bean type with {@link #Grid(Class)}.
*
* <p>
* Every added column sends data to the client side regardless of its
* visibility state. Don't add a new column at all or use
* {@link Grid#removeColumn(Column)} to avoid sending extra data.
* </p>
*
* @param propertyNames
* the property names of the new columns, not <code>null</code>
* @see #addColumn(String)
Expand Down
Loading