Skip to content
This repository has been archived by the owner on Apr 6, 2022. It is now read-only.

Avoid eagerly calling DataProvider.size() when client-side filter cannot be used #222

Closed
Juchar opened this issue Feb 28, 2019 · 9 comments
Assignees
Milestone

Comments

@Juchar
Copy link

Juchar commented Feb 28, 2019

It would be nice to be able to specify when the call to the size() method happens.
Right now, it immediately happens as soon as the dataprovider is set:

private void dataProviderUpdated(boolean forceServerSideFiltering) {
        int size = getDataProvider().size(new Query<>());
        setClientSideFilter(!forceServerSideFiltering && size <= getPageSizeDouble());

        reset();
    }

I would expect it to happen as soon as the dropdown popup is opened for the first time.

This would be especially useful in the situations where multiple ComboBoxes are added inside a form.
Using the current approach the user would have to wait until all the size() calls are done.

Additionally, if the user ust keeps the page open and comes back after some Minutes the size might be obsolete I assume.

Also it sometimes happens that the backend does not expose a proper size() call and unfortunately you have to load the full data right away. Also there it would be useful to be able to decide if the data get's loaded once the user opens the popup.

Best case would be if the call to size() would somehow happen asynchronously and the UI does not have to wait.

Am I overseeing something?

@denis-anisimov
Copy link

I'm not sure I understand the ticket.

It would be nice to be able to specify when the call to the size() method happens

Does it mean that you want to configure somehow at what time size() method should happen ?
I don't understand how you would like to specify this... Please provide the exact API.

At the moment it looks as a too generic request and you may configure this via providing your own ComboBox implementation (I would say this is the only way to solve the ticket in such generic requirement).

So please provide exact request via API contract.

@Legioth
Copy link
Member

Legioth commented Mar 5, 2019

The use case is quite clear: if the same view contains many combo boxes that all depend on relatively expensive database queries to find the size, then rendering performance will suffer for not good reason.

The reason it's implemented to eagerly do size() is because of another optimisation, namely that if there's only a small number of items (and finding them is relatively cheap), then it's more efficient to eagerly send them all to the client so that all operations can happen without additional round trips.

As can be seen from the quoted code snippet, that kind of optimisation is not even considered in cases when filtering cannot happen on the client, which is always the case if a lazy data provider is used.

I'm not sure we'd even need any new API for this case. Just changing the quoted logic so that size() is only run if forceServerSideFiltering is false would avoid at least this particular call to size(). With the current implementation, size() is always called even if the return value might never be used.

@denis-anisimov
Copy link

The ticket title is : Allow configuring when the first call to DataProvider.size() happens.

The optimization of dataProviderUpdated is just one usecase of the requested configuration.
So to be able to fix the ticket one should know what to do: make a way to configure when the first call to DataProvider.size() happens or fix dataProviderUpdated implementation.
I see those two things as different requests.
If this ticket is about dataProviderUpdated optimization then it would be good to change its title.

@Juchar
Copy link
Author

Juchar commented Mar 5, 2019

@Legioth
Thank you, your words do better in explaining what I actually meant.

@denis-anisimov
I am actually not sure if it should be configurable. If it is possible to use the forceServerSideFiltering it would be also fine.


I am just not 100% sure I completely understood the architecture. As far as I can see forceServerSideFiltering will be true as soon as you set a dataprovider (except for a ListDataProvider, where userProvidedFilter will be set explicitely to NO)?

So I assume the following code would be needed for the method mentioned in the original post:

private void dataProviderUpdated(boolean forceServerSideFiltering) {
    if(!forceServerSideFiltering) {
        int size = getDataProvider().size(new Query<>());
        setClientSideFilter(size <= getPageSizeDouble());
    }

    reset(); // Always needed or just if property _clientSideFilter changed?
}

(or better for private void refreshAllData(boolean forceServerSideFiltering) in version 2.1.1)

@Legioth Legioth changed the title Lazy Loading - Allow configuring when the first call to DataProvider.size() happens Avoid eagerly calling DataProvider.size() when client-side filter cannot be used Mar 5, 2019
@Legioth
Copy link
Member

Legioth commented Mar 5, 2019

Client-side filtering is used whenever feasible. It requires these three conditions to be true:

  1. Items have been set using a setItems method or the setDataProvider(ListDataProvider) method. With some other data provider, filtering is done by custom logic in the query and size methods of the data provider.
  2. No custom filtering callback has been configured. The callback is defined as Java and can thus not reasonably be run on the client.
  3. The number of items in the data provider is smaller than the page size. Filtering cannot be applied on the client unless the client has access to all the items.

pekam added a commit that referenced this issue Mar 8, 2019
Now it is not called when using a lazy data provider, because
- the size is used to check if client-side filtering can be used
- lazy data providers always handle filtering in server-side anyway

Fix #222
@pekam pekam self-assigned this Mar 8, 2019
@pekam
Copy link
Contributor

pekam commented Mar 8, 2019

If I understood correctly, @Juchar just wants to avoid this expensive call to size() method.

Like @Legioth pointed out, server-side filtering is forced when using a lazy data provider, so the size-comparison is not needed.

I made the suggested change and marked it to close the issue: #230
With this change, size() should get called only when using in-memory data providers, and in this case the method is cheap to execute.
Let me know if I missed something and it shouldn't close the issue.

pekam added a commit that referenced this issue Mar 11, 2019
Now it is not called when using a lazy data provider, because
- the size is used to check if client-side filtering can be used
- lazy data providers always handle filtering in server-side anyway

Fix #222
@pekam pekam removed the in review label Mar 11, 2019
pekam added a commit that referenced this issue Mar 21, 2019
Now it is not called when using a lazy data provider, because
- the size is used to check if client-side filtering can be used
- lazy data providers always handle filtering in server-side anyway

Fix #222
@pekam pekam added this to the 2.1.3 milestone Mar 21, 2019
pekam added a commit that referenced this issue Mar 21, 2019
Now it is not called when using a lazy data provider, because
- the size is used to check if client-side filtering can be used
- lazy data providers always handle filtering in server-side anyway

Fix #222
@Juchar
Copy link
Author

Juchar commented Mar 25, 2019

Unfortunately the size() call get's still executed using this code:

public class MainView extends VerticalLayout {

  public MainView() {
    ComboBox<String> comboBox = new ComboBox<>("Callbacks");
    comboBox.setDataProvider((filter, offset, limit) -> getItems(), s -> getSize());
    add(comboBox);
  }

  private int getSize() {
    return 2;
  }

  private Stream<String> getItems() {
    return Stream.of("1", "2");
  }
}

As far as I have analysed this call to size() get triggered from the DataCommunicator class, using getDataProviderSize() inside flush(). Unfortunately I was not able to find out why a flush is requested. Any ideas or hints?

Thank you already!

@pekam
Copy link
Contributor

pekam commented Mar 25, 2019

The call for flush is registered by DataCommunicator::requestFlush, which in turn is called already by the DataCommunicator constructor.

I guess that initializing the DataCommunicator could be postponed to the moment when the data is requested for the first time.

I created a new ticket #242

@Juchar
Copy link
Author

Juchar commented Mar 25, 2019

@pekam Thank you for your analysis!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants