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

feat(server): faster folder responses #12214

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

mertalev
Copy link
Contributor

@mertalev mertalev commented Sep 1, 2024

Description

  • Adds separate asset_folders table (many assets to one folder) containing unique parent folders. This ends up being much faster than indexing the main assets table for our access patterns when the library is large.
    • Changes asset creation to first insert into this table
    • Nightly job to remove folders that are no longer used by any assets
  • Sort responses by asset_folder's path and assets's originalFileName, treating numbers as numbers (1, 2, 10 instead of 1, 10, 2)
  • Distinguish between absolute and relative paths (/upload and upload are not the same)
    • Added a few helper functions to handle / and make path manipulation more ergonomic
  • Only query for assets for folders that have assets, using a Symbol in the tree denoting a leaf folder
    • Symbols are unique, don't conflict with string properties and don't appear when listing object keys or entries, making them ideal here

originalPath should ideally be removed to have a single source of truth for the file path and make rows narrower in the assets table. However, in the interest of making downgrading possible and giving us more options to fix any possible bugs, I think it's better to keep it in the short-term.

How Has This Been Tested?

Tested with a large instance containing an external library, storage template assets and a few corrupt images in the upload folder. They all display in the folder sidebar, and navigating each is instant (numbers are response times from the server):

  • 7000-7300ms -> 75ms when getting 35k unique paths for 2m assets in the initial page load
  • 400-600ms -> 8-10ms when going to a folder that contains assets
  • 180-200ms -> 0ms when going to a folder that doesn't contain any assets

Folders and files are in natural sorted order as expected.

The tag page is unchanged.

In draft, pending handling updates to originalPath and adding E2E tests.

@alextran1502
Copy link
Contributor

If you want to update the folders instantly, do we have a job to do that?

@mertalev
Copy link
Contributor Author

mertalev commented Sep 1, 2024

It doesn't handle updates that change originalPath right now (so mainly storage template migration). That will just be modifying the update query and won't be tied to a particular job.

@jrasm91
Copy link
Contributor

jrasm91 commented Sep 1, 2024

This seems to add a lot of edge case handling in the frontend. Can we simplify it by handling more of them server-side?

@mertalev
Copy link
Contributor Author

mertalev commented Sep 1, 2024

We can remove normalizeTreePath since the server response will never have trailing slashes. They won't happen in code either as long as joinPaths is used instead of basic string concatenation.

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Then why do we have this logic here?

@jrasm91
Copy link
Contributor

jrasm91 commented Sep 1, 2024

Do you mind moving the vacuum logic into a separate pull request?

update asset mock

update nightly test

exclude archived assets

update sql

remove vacuuming logic

keep varchar type for filename

set not null
fix typing

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

Successfully merging this pull request may close these issues.

4 participants