0

I'm trying to implement my own VirtualFlow but I'm having some performance issues and I don't understand where the bottleneck could be and what I could do to improve it.
Here's a showcase with two lists, one with simple cells and one with checkboxes too, both have 100_000 items (sorry for the gif quality):

enter image description here

As you can see, I have no issues scrolling through simple cells with just a label, but the list with checkboxes can be quite laggy at the beginning.

To reach such performance (would be much worse otherwise) I memoize my cell factory function so that all cells are ready in memory when needed, I was wondering if there's anything else to make it even faster/smoother.

The code I use to show the cells is:

long start = System.currentTimeMillis();
builtNodes.clear();
for (int i = from; i <= to; i++) {
    C cell = cellForIndex.apply(i);
    Node node = cell.getNode();
    builtNodes.add(node);
}
manager.virtualFlow.container.getChildren().setAll(builtNodes);
long elapsed = System.currentTimeMillis() - start;
System.out.println("Show elapsed: " + elapsed);

I even added this little log, the heavy part seems to be when I call getChildren().setAll(...), both the cell building and the layout are almost immediate

Oh another note on the from and to parameters. When I scroll, I take the scrollbar's value and calculate the first and last visible indexes like this:

public int firstVisible() {
    return (int) Math.floor(scrolled / cellHeight);
}

public int lastVisible() {
    return (int) Math.ceil((scrolled + virtualFlow.getHeight()) / cellHeight - 1);
}

Edit to answer some questions:

Why implementing my own VirtualFlow?

The ultimate goal would be to make my own ListView implementation, in order to do that I also need a VirtualFlow, also I know it's a pretty low level and hard task, but I think it would be a good way to learn more about programming in general and about JavaFX too

Why memoization?

By caching the cell factory results in memory, it makes subsequent scrolls much faster since nodes are already built they just need to be laid out which is fairly easy. The issue is at the start because for some reason the VirtualFlow lags for more complex cells and I can't understand why since the popular library, Flowless, also uses memoization and caches the nodes in memory, but it is very fluent already from the start. JavaFX's VirtualFlow (revised from JFX16) is also very efficient without memoization, but it's much more complex to comprehend

  • 1
    _trying to implement my own VirtualFlow_ why? _some performance issues and I don't understand where the bottleneck could be_ profile, profile, profile .. and _so that all cells are ready in memory_ sounds a perfect recipe into desaster: the whole purpose of a __Virtual__ Flow is to avoid many nodes hanging around. – kleopatra Sep 14 '21 at 10:30
  • @kleopatra Because I want to make my own implementation of a listview, so I also need a VirtualFlow. JavaFX allows making custom controls, even from scratch, so I don't see the problem here. I know the VirtualFlow is a pretty low level concept but that's not a reson why someone should not even try. `Virtual Flow is to avoid many nodes hanging around` I think this is partially correct, a VirtualFlow avoid **showing** unnecessary nodes, only the ones in the viewport, plus this is the same mehcanism Flowless uses, cells are kept in memory, ready to reuse – Alessandro Parisi Sep 14 '21 at 11:48
  • If writing long responses best to edit the question and add a faq section which quotes questions from the comments and adds responses to them (just my opinion). – jewelsea Sep 14 '21 at 15:33
  • @jewelsea done, thanks for the suggestion I'll keep this in mind from now on – Alessandro Parisi Sep 14 '21 at 15:47
  • Are you able to leverage `fixedCellSize` > 0? – trashgod Sep 14 '21 at 15:55
  • repeating: profile your flow to find the bottleneck. There is something wrong/suboptimal with your code that only _you_ can find. When found, distill the problem into a [mcve]. – kleopatra Sep 14 '21 at 16:04
  • @trashgod for simplicity, all cells implement an interface that forces to set a fixed height and width so that I don't have to do too much computation, in future I'd like to implement dynamic cells @kleopatra I profiled it, a lot of times, with a lot of changes and the issue seems to be there, when I do `manager.virtualFlow.container.getChildren().setAll(builtNodes);`. The setAll() method can take up to 20ms at the beginning, but after scrolling and building the cache it goes down to 3ms. As for a reproducible example, it's a lot of code, where should I post it'? – Alessandro Parisi Sep 14 '21 at 16:55
  • This is way out of my league, but... From the [Flowless GitHub](https://github.com/FXMisc/Flowless), "The main feature of a virtual flow is that only the currently visible cells are rendered in the scene." - Are you sure you're not rendering _ALL_ of the cells (or too many) in the scene somewhere? Might be what's causing the issue. It also mentions that it "does not provide higher-level features", perhaps, each of your cells has too many higher level features? This whole thing also reminds me of [RecyclerView](https://developer.android.com/jetpack/androidx/releases/recyclerview) for Android. – Doombringer Sep 14 '21 at 20:00
  • Another thing you might want to check out is with less cells, e.g. 1000 and see if the issue persists. Perhaps having a `CheckBox` and all of its properties is too much for so many cells. Last but not least, you really want to avoid storing stuff in memory that's not really necessary for computations or things that the user can't see - that's how you get Google Chrome. One last thing you could try is to load and unload cells as you're scrolling instead of just `.setAll()` at the beginning and see if it remains all nice and smooth. – Doombringer Sep 14 '21 at 20:00
  • @AlessandroParisi: The API says, "if `fixedCellSize` is greater than zero, we'll use that rather than determine it by querying the cell itself." Did you set the property to match your interface? Your [mcve] can go in your question. This would be a near pointless exercise in Swing; I suspect the same in JavaFX, but [profiling](http://stackoverflow.com/q/2064427/230513) will shed light. – trashgod Sep 14 '21 at 22:13
  • @DoombringerBG yeah, that's what it's written on the Flowless README, and it's true. However, there's a "secret" in Flowless, all Cells are kept in memory, you can test this by having a huge list, scrolling slowly so that all cells are "memoized" and then check the TaskManager. As for the RecyclerView (I know nothing about Android lol), just yesterday I had an idea. Instead of creating new cells everytime, I create just the ones needed to fill the viewport, then on scroll they are just updates instead of changing the children list, it's much faster, I'll post an answer soon. Thanks! – Alessandro Parisi Sep 15 '21 at 13:58

1 Answers1

5

I finally found a solution that is fast and also efficient for memory.
Instead of building new cells every time the view scrolls and update the children list which is super expensive, I build just the needed amount of cells to fill the viewport. On scroll, the cells are laid out and updated. Updating the cells rather than creating and change the children list is much, much faster.

So, the new implementation is very similar to Android's RecyclerView and maybe also to the JavaFX's VirtualFlow, the only difference being that Cells are dumb, quoting from Flowless:

This is the most important difference. For Flowless, cells are just Nodes and don't encapsulate any logic regarding virtual flow. A cell does not even necessarily store the index of the item it is displaying. This allows VirtualFlow to have complete control over when the cells are created and/or updated.

Everyone can extend the interface and define its own logic.
To show a little bit of code:

The Cell interface:

public interface Cell<T> {

    /**
     * Returns the cell's node.
     * The ideal way to implement a cell would be to extend a JavaFX's pane/region
     * and override this method to return "this".
     */
    Node getNode();

    /**
     * Automatically called by the VirtualFlow
     * <p>
     * This method must be implemented to correctly
     * update the Cell's content on scroll.
     * <p>
     * <b>Note:</b> if the Cell's content is a Node this method should
     * also re-set the Cell's children because (quoting from JavaFX doc)
     * `A node may occur at most once anywhere in the scene graph` and it's
     * possible that a Node may be removed from a Cell to be the content
     * of another Cell.
     */
    void updateItem(T item);

    /**
     * Automatically called by the VirtualFlow.
     * <p>
     * Cells are dumb, they have no logic, no state.
     * This method allow cells implementations to keep track of a cell's index.
     * <p>
     * Default implementation is empty.
     */
    default void updateIndex(int index) {}

    /**
     * Automatically called after the cell has been laid out.
     * <p>
     * Default implementation is empty.
     */
    default void afterLayout() {}

    /**
     * Automatically called before the cell is laid out.
     * <p>
     * Default implementation is empty.
     */
    default void beforeLayout() {}
}

The CellsManager class, responsible for showing and updating cells:

    /*
     * Initialization, creates num cells
     */
    protected void initCells(int num) {
        int diff = num - cellsPool.size();
        for (int i = 0; i <= diff; i++) {
            cellsPool.add(cellForIndex(i));
        }

        // Add the cells to the container
        container.getChildren().setAll(cellsPool.stream().map(C::getNode).collect(Collectors.toList()));

        // Ensure that cells are properly updated
        updateCells(0, num);
    }

    /*
     * Update the cells in the given range.
     * CellUpdate is a simple bean that contains the cell, the item and the item's index.
     * The update() method calls updateIndex() and updateItem() of the Cell's interface
     */
    protected void updateCells(int start, int end) {
        // If the items list is empty return immediately
        if (virtualFlow.getItems().isEmpty()) return;

        // If the list was cleared (so start and end are invalid) cells must be rebuilt
        // by calling initCells(numOfCells), then return since that method will re-call this one
        // with valid indexes.
        if (start == -1 || end == -1) {
            int num = container.getLayoutManager().lastVisible();
            initCells(num);
            return;
        }

        // If range not changed or update is not triggered by a change in the list, return
        NumberRange<Integer> newRange = NumberRange.of(start, end);
        if (lastRange.equals(newRange) && !listChanged) return;

        // If there are not enough cells build them and add to the container
        Set<Integer> itemsIndexes = NumberRange.expandRangeToSet(newRange);
        if (itemsIndexes.size() > cellsPool.size()) {
            supplyCells(cellsPool.size(), itemsIndexes.size());
        } else if (itemsIndexes.size() < cellsPool.size()) {
            int overFlow = cellsPool.size() - itemsIndexes.size();
            for (int i = 0; i < overFlow; i++) {
                cellsPool.remove(0);
                container.getChildren().remove(0);
            }
        }

        // Items index can go from 0 to size() of items list,
        // cells can go from 0 to size() of the cells pool,
        // Use a counter to get the cells and itemIndex to
        // get the correct item and index to call
        // updateIndex() and updateItem() later
        updates.clear();
        int poolIndex = 0;
        for (Integer itemIndex : itemsIndexes) {
            T item = virtualFlow.getItems().get(itemIndex);
            CellUpdate update = new CellUpdate(item, cellsPool.get(poolIndex), itemIndex);
            updates.add(update);
            poolIndex++;
        }

        // Finally, update the cells, the layout and the range of items processed
        updates.forEach(CellUpdate::update);
        processLayout(updates);
        lastRange = newRange;
    }

Maybe it's still not perfect, but it works, I still need to test it properly tough for situations like too many cells, not enough cells, the sizes of the container change so the number of cells must be recomputed again...

Performance now:

performance

jewelsea
  • 150,031
  • 14
  • 366
  • 406