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

XCOMMONS-3258: Introduce a cache for ServletEnvironment#getResource #1234

Merged
merged 2 commits into from
Feb 13, 2025

Conversation

michitux
Copy link
Contributor

Jira URL

https://jira.xwiki.org/browse/XCOMMONS-3258

Changes

Description

  • Introduce a dynamically initialized cache to avoid issues with recursive component initialization calls.
  • Add test cases to confirm that the cache works as expected and that cache initialization can do recursive calls for getting resources.

Clarifications

  • As the proposal at https://forum.xwiki.org/t/cache-servletcontext-getresource/16416 and the discussion yesterday didn't bring a real conclusion about introducing a "development" mode, the cache can currently be disabled with the regular CacheControl mechanism that we're already using for other caches. In my opinion, we should discuss improving the cache control mechanism to limit it to users with programming right as this seems to be the easiest solution without introducing any side effects.
  • Without the dynamic initialization, an endless recursion happens because the cache manager loads the configuration from the servlet environment, calling getResource.

Screenshots & Video

No UI changes.

Executed Tests

  • bin/dumbbench --float -p 0.1 -i 100 -- wget -q -o NUL -O NUL --load-cookies cookies.txt "http://127.0.0.1:1700/xwiki/bin/view/NoSpace/NoPage" - the performance seems to be back to what it was approximately in 15.10.16, 103ms vs. 104ms in 15.10.16.
  • LANG=C.UTF-8 mvn clean install -Pdocker,legacy,integration-tests,snapshotModules,quality,distribution,flavor-integration-tests,standalone in xwiki-commons.

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • stable-16.10.x (as this fixes an important performance regression)

* Introduce a dynamically initialized cache to avoid issues with
  recursive component initialization calls.
* Add test cases to confirm that the cache works as expected and that
  cache initialization can do recursive calls for getting resources.
* Increase the default cache capacity to 10k.
* Add some more comments to the test.
@michitux michitux merged commit 51f405d into xwiki:master Feb 13, 2025
@michitux michitux deleted the XCOMMONS-3258 branch February 13, 2025 10:18
Copy link

The backport to stable-16.10.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-stable-16.10.x stable-16.10.x
# Navigate to the new working tree
cd .worktrees/backport-stable-16.10.x
# Create a new branch
git switch --create backport-1234-to-stable-16.10.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 51f405d602712ef32248cfb98b982efd3c4a9e43
# Push it to GitHub
git push --set-upstream origin backport-1234-to-stable-16.10.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-stable-16.10.x

Then, create a pull request where the base branch is stable-16.10.x and the compare/head branch is backport-1234-to-stable-16.10.x.

michitux added a commit that referenced this pull request Feb 13, 2025
…1234)

* Introduce a dynamically initialized cache to avoid issues with
  recursive component initialization calls.
* Add test cases to confirm that the cache works as expected and that
  cache initialization can do recursive calls for getting resources.

(cherry picked from commit 51f405d)
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.

3 participants