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

SLING-11716 ability to cache the results of a caconfig lookup #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

joerghoh
Copy link

  • cache successful resolutions and avoid duplicate resolutions of the same resource (path)
  • updates to a more recent version of the ResourceResolver API
  • the feature is turned off by default

I did a small benchmark (a series of a few hundred requests in a serial manner against an AEM WKND sample page), and found that with this feature enabled there was a cache-hit ratio of approx 75% for the results of caconfig resolutions, resulting in an overall 10% improvement in page rendering time and throughput.

Copy link

sonarcloud bot commented Jan 22, 2024

Copy link

sonarcloud bot commented May 1, 2024

@stefanseifert
Copy link
Member

stefanseifert commented May 17, 2024

lgtm in general, some remarks:

  • making it configurable with default switched off is not really helpful - either we consider this caching safe then we can enable it by default - or if not, what can we do to make it safe
  • when may this caching be harmful? i see two use cases
    • you have a long-running resource resolver e.g. in an OSGi service. this is an anti-pattern, but sometimes required e.g. if you are listing for resource change events.
    • you are actually writing caconfig in your current request and reading it later (most time this is a rare use case)
  • maybe instead of having an OSGi configuration we should add a switch to the caconfig API where you can disable the caching if you are using caconfig in one of this use cases
  • more cosmetic: we should adapt the pattern to build cache keys to use separator chars which are not allowed in JCR paths and not "--" - to be on the safe side.

Copy link
Member

@stefanseifert stefanseifert left a comment

Choose a reason for hiding this comment

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

see previous comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants