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

LazyBookshelf optimizations #3726

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mikiher
Copy link
Contributor

@mikiher mikiher commented Dec 16, 2024

Brief summary

The follwoing changes were made to optimize rendering of LazyBookshelf, especially during scrolling.

  1. Debouncing when scroll rate is high
  2. Wait for entities to be fetched before mounting them
  3. Cleaning up only after scrolling
  4. Calling rebuild directly instead of executeRebuild in settingsUpdated
  5. Adding passive: true option to scroll event listener
  6. Some additional refacroting

Which issue is fixed?

There's no existing issue.

In-depth Description

1. Debouncing when scroll rate is high

When scrolling using the mouse wheel (and, to a lesser extent, by dragging the scrollbar), scrolling events tend to fire very rapidly (up to ~60 times per second), and sometimes for a long while (like when setting the wheel to spin freely on a Logi Mx Master 3 like mine). This creates many redundant handleScroll calls (which, in turn, redundantly fetches and mounts many entities which are outside the visible display).

To counter this, I calculate the scrolling rate on each scroll event, and debounce the handleScroll call for 25 ms if the scroll rate is higher than 5 pixels/ms. This saves a lot of redundant fetching and rendering, and usually during the end of scrolling the rate goes below 5, so the debounding happens mostly in the middle of the scroll period, but not in the end.

Note that a side effect of this is that the bookshelf will go blank during a very fast and long scroll. I think this is acceptable, since you don't gain anything from seeing fast flying entities (the scroll bar is still moving, though, giving you an indication that scrolling happens)

2. Wait for entities to be fetched before mounting them

Up until now, handleScroll called loadPage but didn't wait for it to finish fetching the entities, and mounted empty entities, which caused the book placeholder flicker you sometimes saw. Now we only mount after all the relevant entities have been fetched.

3. Cleaning up only after scrolling

Up until now, every handleScroll call did a cleanup of non-visible entities, with this code:

      this.entityIndexesMounted = this.entityIndexesMounted.filter((_index) => {
        if (_index < firstBookIndex || _index >= lastBookIndex) {
          var el = document.getElementById(`book-card-${_index}`)
          if (el) el.remove()
          return false
        }
        return true
      })

I suspect this cleanup was slowing rendering because of the frequest dom changes. Plus, it only handled book entities, and not other types of entities (which have different ids). I added a debounce of 500 ms here, and fixed the bug.

4. Calling rebuild directly instead of executeRebuild in settingsUpdated

Doing this got rid of 250 ms delay before rebuild, which caused an annoying bookshelf flicker when changing the cover size.

5. Adding passive: true option to scroll event listener

This is supposed to enable smoother scrolling and improve scrolling performance when the listener doesn't need to call preventDefault.

6. Some additional refacroting

While doing all of this, I made a number of refactoring changes:

  • rewrote rebuild, and got rid of remountEntities
  • pagesLoaded now contains fetchEntities promises instead of booleans
  • fetchEntities is now always called only from loadPage

Things yet to be fixed

While testing this, I found that the server has very different response times for page fetches for different sortBy values. When sorting by Added at or Title, server response time is usually below 100 ms for a page fetch request. However when sorting by Size, for example, the response time tends to go up significantly, and when scrolling fast, the server is handling a number of requests simultaneously, and then it behaves really bad (all simultaneous requests take 5 or more seconds) - we've already seen that Sequelize/SQlite handles concurrent requests very poorly, and I suspect that this is the issue here as well.

I suspect that the poor performance on certain sortBy values is due to lack of proper indices, but will need to investigate further.

How have you tested this?

  • Lots of scrolling, using different scrolling devices...
  • Moving between cover sizes
  • Moving between different sortBy and filterBy values
  • Tested on all pages using LazyBookshelf (Library, Series, Collections, Playlists, Authors).
  • Tested on library with many books, series, and authors (~4k books)

@mikiher mikiher marked this pull request as ready for review December 16, 2024 17:43
@nichwall
Copy link
Contributor

I'm not sure if this is the case for the library view of sorting by size, but I noticed that some endpoints are calculating the size of the item based on the size of every file instead of using the size column.

I opened an issue that discussed this a bit here (noticed that a lot of my items still have a NULL size): #3673

If sorting by size is using the size column, then the slowdown would be due to improper indices like you said.

@advplyr
Copy link
Owner

advplyr commented Dec 16, 2024

Up until now, handleScroll called loadPage but didn't wait for it to finish fetching the entities, and mounted empty entities, which caused the book placeholder flicker you sometimes saw. Now we only mount after all the relevant entities have been fetched.

It was intentional to show the blank cards to indicate that an item is there and it just hasn't been loaded yet. Like a skeleton component.

The flickering placeholder image is a bug that didn't come up until later and I haven't figured out why that happens.


In LazyBookCard.vue the following code will hide img until libraryItem is set.

When libraryItem is set bookCoverSrc will be the URL to the item cover and not the placeholder cover URL.

After the libraryItem is set and before the image loads the title is shown.

<div cy-id="titleImageNotReady" v-show="libraryItem && !imageReady" aria-hidden="true" class="absolute top-0 left-0 w-full h-full flex items-center justify-center" :style="{ padding: 0.5 + 'em' }">
<p :style="{ fontSize: 0.8 + 'em' }" class="text-gray-300 text-center">{{ title }}</p>
</div>
<!-- Cover Image -->
<img cy-id="coverImage" v-show="libraryItem" :alt="`${displayTitle}, ${$strings.LabelCover}`" ref="cover" aria-hidden="true" :src="bookCoverSrc" class="relative w-full h-full transition-opacity duration-300" :class="showCoverBg ? 'object-contain' : 'object-fill'" @load="imageLoaded" :style="{ opacity: imageReady ? 1 : 0 }" />

I don't see where in that code the placeholder cover image would be shown. I haven't found a way to reliably reproduce the placeholder image flickering.


I think we should show something there while the items are loading. I like the skeleton placeholder components better than using a loading indicator. Some users have really slow server responses and could see a blank screen for several seconds.

@advplyr
Copy link
Owner

advplyr commented Dec 16, 2024

It was supposed to look this while they were fetching.

image

I believe the first iteration also had a loading spinner on each cover that I removed

@mikiher
Copy link
Contributor Author

mikiher commented Dec 17, 2024

Acknowledged. Working on it.

@mikiher
Copy link
Contributor Author

mikiher commented Dec 18, 2024

OK, let me address the placeholder flicker bug first.

You were right that my PR didn't address the root cause of that bug. I'll argue, though, that my PR does eliminate this bug inadvertantly (explanation later below).

I'm able to reproduce the bug almost every time I try, by scrolling the mouse wheel very quickly and then stopping it abruptly. This is more visible when some of the slower sortBy values is used (e.g. size or duration).

Here's an example, using the current head code. You can see the flicker at around 00:08.

Screen.Recording.2024-12-18.091510.mp4

I think I now understand why this bug is happening. It is a combination of two things:

  1. Mounting an empty card and then later (when the entity is fetched), calling setEntity on that card.
  2. Non-atomic Vue reacitivity behavior.

It all revolves around this line in LazyBookCard:

<img cy-id="coverImage" v-show="libraryItem" ref="cover" :src="bookCoverSrc" class="relative w-full h-full transition-opacity duration-300" :class="showCoverBg ? 'object-contain' : 'object-fill'" @load="imageLoaded" :style="{ opacity: imageReady ? 1 : 0 }" />

But let me use a use a more simplifed version to illustrate what I think is happening:

<img v-show="libraryItem"  :src="bookCoverSrc" />

So here's what I believe is happening, chronologically.

  1. An empty LazyBookCard is mounted (this happens in bookshelfCardsHelpers.js:mountEntityCard, when a card is mounted before the entity is fetched from the server, which can happen currently in the code, prior to my PR). At this point, libraryItem is null so the the item is hidden, and bookCoverSrc has the value of /book_placeholder.jpg. So the element that is loaded to the dom at this point is:
<img style="display: none;"  src="/book_placeholder.jpg" />
  1. setEntity is called after the entity is fetched from the server (this happens in LazyBookshelf.js:fetchEntities). Now libraryItem becomes non-null, and Vue reacitivity kicks in, but importantly I believe this happens in two stages:
    1. v-show now has a truthy value, so the element becomes visible:
      <img src="/book_placeholder.jpg" />
    2. The bookCoverSrc computed property reacts to change in libraryItem and now the element becomes:
      <img src="/path/to/real/cover.jpg" />
    The flicker is caused (it seems), by the fact that the changes i and ii do not happen at the same time. First the (already existing!) element becomes visible, with src still pointing at the placeholder, and the dom is updated and rendered, and we see the placeholder for a short time, and then bookCoverSrc is re-computed, the value of src changes, and the dom is again updaed and rendered, and now we see the real cover. Obviously, this depends on Vue/Browser timing, and therefore might not be reproduced consistently. But I think it is a reasonable explanation to what we see, and the fact that the placeholder appears very shortly, or not at all.

Now, if we were to change v-show to v-if, one may assume that the flicker would not appear, since before the setEntity call the img element doesn't exist, and it would only be ready to be rendered when the src attribute has been computed for the first time, which would only happen with the non-null null libraryItem value. So at no point in time does src point at the placeholder url, and therefore — no flicker.

I tested this, and indeed, the flicker disappeared!

Screen.Recording.2024-12-18.220906.mp4

Now going back to my PR, this is why I claimed it inadvertantly fixed the issue — since it doesn't mount empty cards, then at no point is the card's img element point at the placeholder, and therefore no flicker.

Let me know if you have any questions/comments about this point.

I'll address the other points separately.

@advplyr
Copy link
Owner

advplyr commented Dec 18, 2024

That makes sense, thanks for digging into it.
Are you adding back in the skeleton card since now we know how to prevent the flicker?

@mikiher
Copy link
Contributor Author

mikiher commented Dec 18, 2024 via email

@mikiher
Copy link
Contributor Author

mikiher commented Dec 21, 2024

OK, I tried to resolve this without mounting empty entity cards — I think it's wasteful and expensive to load empty entities that will not end up being shown, and it also shows strange artefacts like (like size: 0 bytes) while they're being fetched.

Instead, I put card skeletons into the empty shelves. Those are much lighter than cards and can be pre-loaded (like the empty shelves).

I also fixed the root cause of the placeholder flicker.

PTAL, and let me know what you think.

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

Successfully merging this pull request may close these issues.

3 participants