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

perf: optimize the permission check to use an in-memory object #889

Open
wants to merge 1 commit into
base: 1.x
Choose a base branch
from

Conversation

dkarlovi
Copy link

@dkarlovi dkarlovi commented Sep 26, 2024

Closes #886.

Comparison current and new, fetching 100 products in a listing:
Comparison-from-Untitled-to-Untitled-Blackfire-09-26-2024_03_04_PM

Copy link

github-actions bot commented Sep 26, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@dkarlovi
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

Copy link

sonarcloud bot commented Sep 26, 2024

@mattamon
Copy link
Contributor

mattamon commented Oct 4, 2024

@dkarlovi tested it and it works fine. My only concern is that the $cacheKey does not get invalidated if you save the configuration. If the permissions change you need to call cache:clear to make it work.
Could you also please invalidate the key on config save.

Also maybe change the key to datahub_workspace_permissions_ to make it more specific.

@dkarlovi
Copy link
Author

dkarlovi commented Oct 4, 2024

@mattamon is there a cache tag which gets cleared on workspace save, like asset_ID or object_ID do when they're saved? We can then add a tag to the cache item and it gets automagically invalidated.

@mattamon
Copy link
Contributor

mattamon commented Oct 4, 2024

@mattamon is there a cache tag which gets cleared on workspace save, like asset_ID or object_ID do when they're saved? We can then add a tag to the cache item and it gets automagically invalidated.

Hmm, I checked and it does not seem like it. The configs are in general not cached, other than by the locationaware dao.
But this is a static cache.

@fashxp do you have a tag in mind? Otherwise I would introduce a tag and change the Configuration Dao save method to clear that tag.

@dkarlovi
Copy link
Author

dkarlovi commented Oct 4, 2024

The configs are in general not cached

IMO that's also an opportunity to improve: why aren't they cached, wouldn't the DAO do that by default? 🤔 This would mean we'd get cache invalidation for free here just by tagging.

@mattamon
Copy link
Contributor

mattamon commented Oct 4, 2024

The configs are in general not cached

IMO that's also an opportunity to improve: why aren't they cached, wouldn't the DAO do that by default? 🤔 This would mean we'd get cache invalidation for free here just by tagging.

Yeah I thought the same.
Like I wrote they are kind of cached here https://github.com/pimcore/pimcore/blob/11.x/lib/Model/Dao/PimcoreLocationAwareConfigDao.php but in a static cache.

@fashxp
Copy link
Member

fashxp commented Oct 4, 2024

@fashxp do you have a tag in mind? Otherwise I would introduce a tag and change the Configuration Dao save method to clear that tag.

nope, no tag in mind... let's introduce a new one.

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.

[Improvement][Perf]: WorkspaceHelper::isAllowed creating hundreds of database queries for listings
5 participants