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.

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

Peformance issues dynamically updating widget heirarchy #683

Open
TreyBoudreau opened this issue Dec 28, 2020 · 7 comments
Open

Peformance issues dynamically updating widget heirarchy #683

TreyBoudreau opened this issue Dec 28, 2020 · 7 comments

Comments

@TreyBoudreau
Copy link

TreyBoudreau commented Dec 28, 2020

Having resolved my issue around knowing the clientHeight, I ended up with this (borrowing heavily from the TreeWidget):

from pscript import undefined, window, RawJS
from flexx import flx

class VirtualListItem(flx.Widget):
    """
    An empty container widget to manage the rowIndex property, used to position
    the virtual list items using absolute coordinates.
    """
    rowIndex = flx.IntProp(-1, settable=False, doc="""
    The index of this item's row in the table. It matches the index passed to
    VirtualItemGenerator.generate_item().
    """)
    pass

class VirtualListWidget(flx.Widget):
    """
    A Widget used to structure information in a list. To avoid creating large
    DOM heiararchies the VirtualListWidget instantiates only those items
    actually visible in its bounding rectangle.

    TODO: cache the objects and render some both above and below for nicer
    scrolling.

    The ``node`` of this widget contains a
    `<div> <https://developer.mozilla.org/docs/Web/HTML/Element/div>`_
    with some child elements and quite a bit of CSS for rendering.
    
    **Style**

    You may fully style this widget with CSS, using the following CSS classes:

    TODO

    """

    CSS = """

    /* ----- Virtual List Widget Mechanics ----- */

    .flx-VirtualListWidget {
        height: 100%;
        overflow-y: scroll;
        overflow-x: hidden;
        cursor: default;
        scrollbar-gutter: always;
    }

    .flx-VirtualListWidget .flx-VirtualListItem {
        display: inline-block;
        margin: 0;
        padding-left: 2px;
        width: 100%;
        position: absolute;
        user-select: none;
        -moz-user-select: none;
        -webkit-user-select: none;
        -ms-user-select: none;
    }

    """
    scrollTop = flx.IntProp(0, settable=True, doc="""
    """)


    def init(self, data_source):
        if not isinstance(data_source, VirtualItemGenerator):
            raise TypeError("data_source must derive from VirtualItemGenerator")
        self._clientHeight = 0
        self._data_source = data_source
        self._addEventListener(self.node, 'scroll', self.scrolled, 0)
        # The scroller_pane causes the scroll bars to appear.
        # TODO: If total_height() changes, we'll need to update the size.
        self.scroller_pane = flx.Widget(
            minsize=(0, self._data_source.total_height()),
            maxsize=(0, self._data_source.total_height())
        )

    @flx.emitter
    def scrolled(self, event):
        return dict(scrollTop=event.currentTarget.scrollTop)

    @flx.action
    def scroll_to(self, newTop):
        self._mutate_scrollTop(newTop)

    def manage_children(self):
        """ Make sure we have just enough children to fill the view. """
        # first ensure we have a valid height
        if self._clientHeight == 0:
            return
        msg = "manage_children>"
        RawJS("console.info(msg)")
        # calculate the index of the first and last items in the view
        top_limit = self.outernode.scrollTop
        bot_limit = top_limit + int(self.outernode.clientHeight)
        top_index = self._data_source.index_at_offset(top_limit)
        bot_index = self._data_source.index_at_offset(bot_limit)
        lowest_index = bot_index
        highest_index = top_index
        # dispose any items which have scrolled completely out of the view
        for child in self.children:
            if child.outernode.classList.contains("flx-VirtualListItem"):
                ri = child.rowIndex
                if ri < lowest_index:
                    lowest_index = ri
                if ri > highest_index:
                    highest_index = ri
                if ri < top_index or ri > bot_index:
                    msg = f"deleting {ri}"
                    RawJS("console.info(msg)")
                    child.dispose()
        # create any missing items
        for index in range(top_index, bot_index+1):
            if index < lowest_index or index > highest_index:
                msg = f"creating {index}"
                RawJS("console.info(msg)")
                with VirtualListItem(parent=self, rowIndex=index, style=f"top: {self._data_source.offset_of(index)}px") as item:
                    # item not strictly required, but it saves a tiny amount of
                    # time during construction
                    self._data_source.generate_item(item, index)

        msg = "manage_children<"
        RawJS("console.info(msg)")

    def _render_dom(self):
        # just for tracking purposes
        msg = "render_dom()"
        RawJS("console.info(msg)")
        return super()._render_dom()

    @flx.reaction('scrollTop', mode='greedy')
    def do_scroll(self, *events):
        msg = "do_scroll()"
        RawJS("console.info(msg)")
        self.manage_children()

    @flx.reaction('size', mode='greedy')
    def __our_size_changed(self, *events):
        """
        Our viewport size has changed. We receive one or more of them during
        layout, but before we actually know our clientHeight. We ignore these
        values unless our clientHeight changes.
        """
        if self._clientHeight != self.outernode.clientHeight:
            self._clientHeight = self.outernode.clientHeight
            self.manage_children()

    @flx.reaction('scrolled', mode='greedy')
    def on_scroll(self, *events):
        msg = "on_scroll()"
        RawJS("console.info(msg)")
        ev = events[-1]
        self.scroll_to(ev.scrollTop)

class VirtualItemGenerator:
    """
    Abstract base class to enforce the API for generating list items.
    """
    def len(self):
        """ Returns the number of items in the list. """
        return 0

    def total_height(self):
        """ Returns the total height of the list in pixels. """
        return 0

    def index_at_offset(self, offset):
        """ Return the index of the element at the specified offset. """
        return 0

    def generate_item(self, parent, index):
        """ Generate a DOM node for the element at the specified index. """
        return None

class FixedHeightGenerator(VirtualItemGenerator):
    """
    Encapsulate the logic for fixed height items.
    Derive from this class and implement generate_item().
    Items with variable height require custom logic, but they will work.
    """
    def __init__(self, item_height, count):
        self._item_height = item_height
        self._count = count

    def len(self):
        return self._count

    def total_height(self):
        return self._item_height * self._count

    def index_at_offset(self, offset):
        return min(window.Math.floor(offset / self._item_height) + 1, self._count) - 1

    def offset_of(self, index):
        return self._item_height * index

class FakeItemGenerator(FixedHeightGenerator):
    def __init__(self, height, count):
        super().__init__(height, count)

    def generate_item(self, parent, index):
        return flx.Label(parent=parent, flex=1, text=f"Item #{index}")

class Example(flx.Widget):

    CSS = '''
    .flx-VirtualListWidget {
        background: #000;
        color: #afa;
    }
    '''

    def init(self):

        with flx.HSplit(minsize_from_children=False):
            self.label = flx.Label(flex=1, style='overflow-y: scroll;')
            self.list = VirtualListWidget(FakeItemGenerator(20, 1000), flex=1)

    @flx.reaction('list.scrolled')
    def on_event(self, *events):
        for ev in events:
            text = str(ev.scrollTop)
            self.label.set_html(text + '<br />' + self.label.html)


if __name__ == '__main__':
    #m = flx.launch(Example)
    #flx.run()
    a = flx.App(Example)
    a.serve()
    flx.start()

This code works, but actually updating the display takes quite a bit of time. Given that updating one or two items (triggered via scroll-wheel) happens quite quickly, but scrolling an entire page (via clicking at an arbitrary location in the scroll bar) takes on the order of a second for a window with about 100 visible items, I suspect the destruction/construction overhead.

I think I went about this in a Flexx-friendly way, but I'd welcome any suggestions for improving performance.

@almarklein
Copy link
Member

At first glance this looks technically correct. A small tip could be that the console.log calls may affect performance too, partly because these messages are send to the server as well.

From a more fundamental point of view, you could ask whether using Flexx widget objects for the leaf information (the objects being created and deleted all the time). They certainly may introduce overhead.

Three alternatives:

  • You may use _render_dom to produce (virtual) DOM nodes. Because these do not have the per-widget overhead, this may well be faster. This would be my suggestion if the current approach is not performant enough.
  • You could not use Flexx for the leaf nodes, but JS with document.createElement etc. This requires knowledge of the respective DOM API, and I actually don't expect it to be (much) faster then the above. In fact, the above uses a virtual DOM which is diffed with the real DOM, so it may in fact be faster.
  • You could use a <canvas> to draw the leaf items. This is a completely different API, but will in most cases probably be the fastest solution.

@TreyBoudreau
Copy link
Author

I looked into using _render_dom() initially, but I didn't find a way to trigger rebuilding the DOM in response to scrolling. After some digging, I think I can call Widget.__render() from one of my reactions, but given the double leading underscores in the name, it feels like I'll create a dependency on a private implementation detail. If this doesn't work (or I shouldn't depend on it), how do you recommend triggering the DOM rebuild?

Based on this note in class Widget:

        # TODO: we listen to a lot of events which is unncessary in a lot of cases.
        # Maybe make it possible (with a class attribute?) to configure this

I hacked in

    default_events = event.BoolProp(True, settable=True, doc="""
        If true, add event handlers for mouse and touch events.
        """)

...

    def _init_events(self):
        if not self.default_events:
            return
...

This helps some, as not hooking up 15 events for 2 * visible_items adds up to quite a bit of overhead. This semi-approximates using _render_dom() by reducing the per-widget overhead. I don't know if this represents the "right" way resolve the TODO or not, and I'd rather not modify the library core locally if I can help it.

@TreyBoudreau
Copy link
Author

It seems I can't call self.__render() because PScript mangles the name during translation in with_prefix():

[E 11:52:40 flexx.app] JS: TypeError: this._VirtualListWidget__render is not a function

At the moment, I don't see any way around this without hacking something like this into class Widget:

    def render_hammer(self):
        self.__render()

After doing so and overloading _render_dom(), it works great: no more performance issues.

I have yet to figure out what property or properties Widget.__render() reacts to, so I don't know how to trigger rendering any other way :-(

@almarklein
Copy link
Member

but I didn't find a way to trigger rebuilding the DOM in response to scrolling

The _render_dom method is being called from an implicit reaction. This means that you can access properties in the body of _render_dom, and Flexx will know what properties were accessed, and make sure to call _render_dom again when any of these change.

In this case, create a property that gets changed whenever you scroll, and access that property in _render_dom.

@TreyBoudreau
Copy link
Author

Ah, I actually have to reference all properties I might use the first time Flexx calls _render_dom()!
If the first call to _render_dom() does not reference Widget.size because of a if statement, Flexx won't call _render_dom() when it changes.
I had assumed Flexx performed it magic in the PScript translation, but in fact it happens in Property.__get__().

@almarklein
Copy link
Member

Yes, good point.

@TreyBoudreau
Copy link
Author

As a mostly static-compilation guy, I hold the "Introspect All The Things" implementation of Flexx in a certain amount of awe (and a tiny amount of horror ;-) Thus I pay you the highest compliments I know:

  1. I wish I'd thought of that.
  2. I plan on stealing the ideas for my future Python code.

Thanks for the help!

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

No branches or pull requests

2 participants