Skip to content

fix(taskprocessing): More caching - #50331

Merged
marcelklehr merged 6 commits into
masterfrom
fix/perf/cache-avilable-taskt-types
Jan 25, 2025
Merged

fix(taskprocessing): More caching#50331
marcelklehr merged 6 commits into
masterfrom
fix/perf/cache-avilable-taskt-types

Conversation

@marcelklehr

@marcelklehrmarcelklehr commented Jan 23, 2025

Copy link
Copy Markdown
Member

Summary

Shave off some miliseconds.

Checklist

@marcelklehr
marcelklehrforce-pushed the fix/perf/cache-avilable-taskt-types branch from 59e1551 to 258df9eCompareJanuary 23, 2025 08:54
ICacheFactory $cacheFactory,
) {
$this->appData = $appDataFactory->get('core');
$this->cache = $cacheFactory->createLocal('task_processing::');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

distributed cache would be good here too, it falls back to local cache if unavailable

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I believe we can't use distributed cache here because it uses json_encode to serialize the values and that breaks stuff

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

apcu will serialize faithfully, afaik

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can probably handle the json_encode ourselves and pass the validated result only to the cache but feel free to use local only, won't affect it much

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

At least the result of getAvailableTaskTypes( should be string-array only? So should be totally fine?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Small comment regarding local vs distributed cache.

Local cache should typically be better for performance because it has lower latency. The distributed cache leaves the machine, at least for clustered setups with a dedicated Redis node.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

At least the result of getAvailableTaskTypes( should be string-array only?

It's not, sadly

@marcelklehrmarcelklehr changed the title fix(taskrpocessing): Cache result of getAvailableTaskTypes between requestsfix(taskprocessing): More cachingCache result of getAvailableTaskTypes between requestsJan 23, 2025
@marcelklehrmarcelklehr changed the title fix(taskprocessing): More cachingCache result of getAvailableTaskTypes between requestsfix(taskprocessing): More cachingJan 23, 2025
Comment threadlib/private/TaskProcessing/Manager.php
Comment threadlib/private/TaskProcessing/Manager.php Outdated
Comment threadlib/private/TaskProcessing/Manager.php
@marcelklehr

Copy link
Copy Markdown
MemberAuthor

/backport to stable31

@marcelklehr

Copy link
Copy Markdown
MemberAuthor

/backport to stable30

@marcelklehr

Copy link
Copy Markdown
MemberAuthor

Cypress failure seems unrelated

cc @AndyScherzinger

@marcelklehr

Copy link
Copy Markdown
MemberAuthor

Together with nextcloud/app_api#500 this shaves 150ms off getNextScheduledTask on my machine

@marcelklehr
marcelklehrforce-pushed the fix/perf/cache-avilable-taskt-types branch from 0c393bf to 028e6e2CompareJanuary 24, 2025 09:40
@oleksandr-nc

Copy link
Copy Markdown
Contributor

Together with nextcloud/app_api#500 this shaves 150ms off getNextScheduledTask on my machine

Quite a serious figure, excellent optimization!

@marcelklehr
marcelklehrforce-pushed the fix/perf/cache-avilable-taskt-types branch from 028e6e2 to fa4bd44CompareJanuary 24, 2025 11:19
marcelklehrand others added 5 commits January 24, 2025 16:45
…quests
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
…well
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
Co-authored-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@marcelklehr
marcelklehrforce-pushed the fix/perf/cache-avilable-taskt-types branch from fa4bd44 to a610002CompareJanuary 24, 2025 15:45
@marcelklehr
marcelklehr merged commit b0d1f2c into masterJan 25, 2025
@marcelklehr
marcelklehr deleted the fix/perf/cache-avilable-taskt-types branch January 25, 2025 08:11
@AndyScherzingerAndyScherzinger added this to the Nextcloud 32 milestone Jan 25, 2025
@nextcloud-botnextcloud-bot mentioned this pull request Aug 19, 2025
Sign up for freeto 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.

8 participants

@marcelklehr@oleksandr-nc@nickvergessen@AndyScherzinger@ChristophWurst@julien-nc@kyteinsky@skjnldsv