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

Cache path by id #43471

Merged
merged 3 commits into from
Mar 5, 2024
Merged

Cache path by id #43471

merged 3 commits into from
Mar 5, 2024

Conversation

icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Feb 9, 2024

Cache fileid to path mapping.

The fact that a single fileid can have multiple paths complicates attampts to cache this information, however since most usages of getById only use a single item of the result, and always the same one (whether or not this is correct for that usage is another question 🙈). We can provide a new api that only returns a single result which makes caching easier.

We can get away with using a local cache without doing any active cache invalidation by checking that the node at the path we cached still has the correct fileid.

  • add getFirstNodeById to get a single node for id regardless of how many nodes with that id the user has
  • switch over all calls to getById that always use the first node to the new api
  • add caching to getFirstNodeById

I would recommend reviewing this PR commit-by-commit as most of the changes by LOC are mechanical changes to adjust to the new api.

@icewind1991 icewind1991 force-pushed the cache-path-by-id branch 2 times, most recently from ed9cc7d to a2846be Compare February 9, 2024 10:58
@icewind1991 icewind1991 force-pushed the cache-path-by-id branch 3 times, most recently from 6baf20a to 57e7266 Compare February 16, 2024 11:23
lib/private/Files/Node/Root.php Fixed Show fixed Hide fixed
lib/private/Files/Node/Root.php Fixed Show fixed Hide fixed
@icewind1991 icewind1991 force-pushed the cache-path-by-id branch 2 times, most recently from 0a36121 to 68ad797 Compare February 16, 2024 12:03
@icewind1991 icewind1991 marked this pull request as ready for review February 16, 2024 12:57
@icewind1991 icewind1991 requested review from a team, ArtificialOwl, blizzz and nfebe and removed request for a team February 16, 2024 12:57
@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Feb 16, 2024
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 23, 2024
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

👍

lib/private/Files/Node/LazyFolder.php Outdated Show resolved Hide resolved
@icewind1991 icewind1991 force-pushed the cache-path-by-id branch 2 times, most recently from dbb9170 to 3dfef53 Compare February 29, 2024 15:52
…des for the id

this should be enough in most(?) cases and makes efficient implementation and caching easier

Signed-off-by: Robin Appelman <robin@icewind.nl>
…rstNodeById

Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991 icewind1991 added 3. to review Waiting for reviews performance 🚀 and removed 2. developing Work in progress labels Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants