Repository navigation
Conversation
630ab2d to
c5079fa
Compare
Preload the tags of a folder's children in one query, sort them once and reuse the serialized <nc:system-tag> element for every file instead of rebuilding it. The response stays byte-identical. Signed-off-by: ふぁ <yuki@yuki0311.com> Assisted-by: ClaudeCode:claude-fable-5-1
c5079fa to
56a1687
Compare
fa0311
changed the title
| public function set(string $key, string $fragment): void { | ||
| $this->fragments[$key] = $fragment; | ||
| } | ||
| } |
There was a problem hiding this comment.
We do not need a class for this, it’s just an array.
There was a problem hiding this comment.
Removed. The plugin keeps the serialized elements in a plain array keyed by tag id, and SystemTagList only receives the fragments of its tags in list order and writes them with writeRaw().
While at it I dropped the separate XMLWriter: the fragments are now built with a writer from the server's XML service, the same way PaginatePlugin::getXmlRendererGenerator() renders entries, so there is a single serializer and the output stays byte-identical.
| private array $tags, | ||
| ISystemTagManager $tagManager, | ||
| ?IUser $user, | ||
| private SystemTagFragmentCache $fragments = new SystemTagFragmentCache(), |
There was a problem hiding this comment.
How big does it get? Do we want to use a CappedMemoryCache here?
There was a problem hiding this comment.
Each fragment is 120–160 bytes and there is one per distinct tag seen in the request, freed with the plugin instance at the end of the request. Even 10k distinct tags in one listing would be about 2 MB, less than the ISystemTag objects the plugin already caches for the same ids.
A CappedMemoryCache (512 entries by default) would evict exactly on the large folders where this matters, so I left it uncapped.
…riter Address review: keep the per-file tag lists sorted and visibility-filtered from the preload, build each system-tag element once with the response writer instead of a separate XMLWriter, and drop the cache class, the lazy getTagsForFile() path and the per-file canUserAssignTag() calls. Equal names are ordered by tag id. Signed-off-by: ふぁ <yuki@yuki0311.com> Assisted-by: ClaudeCode:claude-fable-5-1
… folders SystemTagList takes the serialized element of each tag in the same order as the tags, so a missing fragment cannot slip through as a warning. A folder that was already preloaded as a child still preloads its own children, which the nested-folder test now covers. The fragment writer is created per preload instead of being kept on the plugin. Signed-off-by: ふぁ <yuki@yuki0311.com> Assisted-by: ClaudeCode:claude-fable-5-1
|
Updated with two commits addressing the review. |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
This branch has not been deployed
2 participants
Summary
Listing a folder with
nc:system-tagsrequested gets slow once files carry many tags.SystemTagPluginalready preloads the tag mappings of the whole folder, but everything after that still ran per file: fetching the tag objects (onegetTagsByIds()for every file that brought a new tag), sorting withnaturalSortCompare(), checking visibility, and writing every<nc:system-tag>element through the Sabre writer again.Now the plugin loads the distinct tags once, sorts them once and keeps the natural-sort rank per tag, so a file's tags are ordered by bucketing instead of
usort.SystemTagListserializes a tag once and reuses the fragment viawriteRaw(); the fragments live in a request-scopedSystemTagFragmentCache. Tags with the same name keep their previous relative order, and the response is byte-identical (checked withcmpon a 55 MB response with<,>,&and"in tag names).On a folder with 10k files and ~385k tag mappings,
PROPFINDwithoc:fileid+nc:system-tagsgoes from 6.2 s / ~700 queries to 2.4 s / ~40 queries; the full Files app property set from 8.2 s to 4.1 s.Checklist
3. to review, feature component)stable32)AI (if applicable)