perf(core): optimize dependency sorting and relation manager deletion#1501
perf(core): optimize dependency sorting and relation manager deletion#1501fanti1 wants to merge 1 commit into
Conversation
WalkthroughThe changes update three independent code paths: relation multi-delete now bulk-loads matching models, list pagination uses the query builder’s pagination count, and plugin dependency sorting precomputes filtered dependency lists while tracking resolved plugins with a key set. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
fanti1
force-pushed
the
perf/core-optimizations
branch
from
5b811ee to
7747915
Compare
|
Run the performance numbers with 1, 10, and 100 plugins please, 200 as your starting point is absolutely ludicrous. |
fanti1
force-pushed
the
perf/core-optimizations
branch
from
7747915 to
5b811ee
Compare
Here are the benchmark numbers on small scales (1, 10, and 100 plugins in a deep worst-case dependency chain, averaged over 10,000 runs): |
|
@fanti1 while I appreciate the effort making changes to the plugin loading and dependency resolution logic is very risky and highly likely to break things. Before investing time into analyzing these changes (especially when there's no test coverage) I would like to know what the real world impact of the changes will be. 200 plugins is not a realistic amount of plugins to be present in the vast majority of real projects. It also makes it harder to review these sorts of changes when two unrelated changes are bundled together in the same PR. Again, I appreciate the effort and if this is a safe change that improves Winter I'll be happy to merge it. Maintaining an open source project is a fine balance between optimizing for speed, stability, and maintainability. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/system/classes/PluginManager.php (1)
982-1003: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrecomputation logic is correct; optional readability extraction.
The hoisted
$dependenciesMapbuild correctly mirrors the resolve → normalize → replacement-alias → filter pipeline needed for the traversal below, and moving it outside thewhileloop is exactly the optimization described in the PR. Given this segment was flagged as high complexity, consider extracting the per-dependency resolution (normalize + replacement lookup) into a small private helper (e.g.resolveDependencyCode(string $depend): string) to make the closure's intent self-documenting.♻️ Optional extraction
- $depends = array_map(function ($depend) { - $depend = $this->getNormalizedIdentifier($depend, true); - - if (isset($this->replacementMap[$depend])) { - return $this->replacementMap[$depend]; - } - - return $depend; - }, $depends); + $depends = array_map([$this, 'resolveDependencyCode'], $depends);protected function resolveDependencyCode(string $depend): string { $depend = $this->getNormalizedIdentifier($depend, true); return $this->replacementMap[$depend] ?? $depend; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/system/classes/PluginManager.php` around lines 982 - 1003, Extract the normalize-and-replacement logic from the dependency mapping closure in PluginManager into a small private/protected helper such as resolveDependencyCode(string $depend): string. Have the closure call this helper while preserving the existing filtering and dependenciesMap behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/backend/behaviors/RelationController.php`:
- Around line 1246-1248: The relation lookup in the management flow must enforce
the current parent relation instead of querying the base model directly. Update
the `$objects` query near `$foreignKeyName` to use `$this->relationObject`, its
qualified key, and the existing deferred-binding/parent constraints; apply the
same correction in `onRelationManageRemove()`.
---
Nitpick comments:
In `@modules/system/classes/PluginManager.php`:
- Around line 982-1003: Extract the normalize-and-replacement logic from the
dependency mapping closure in PluginManager into a small private/protected
helper such as resolveDependencyCode(string $depend): string. Have the closure
call this helper while preserving the existing filtering and dependenciesMap
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee900e23-5e02-4c00-be9f-8f871fbc0ccd
📒 Files selected for processing (3)
modules/backend/behaviors/RelationController.phpmodules/backend/widgets/Lists.phpmodules/system/classes/PluginManager.php
| $foreignKeyName = $this->relationModel->getKeyName(); | ||
| $objects = $this->relationModel->whereIn($foreignKeyName, $checkedIds)->get(); | ||
| foreach ($objects as $obj) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if similar unconstrained IDOR vulnerabilities exist in `onRelationManageRemove`.
rg -n -A 20 "public function onRelationManageRemove" modules/backend/behaviors/RelationController.phpRepository: wintercms/winter
Length of output: 994
🏁 Script executed:
sed -n '1228,1265p' modules/backend/behaviors/RelationController.php
printf '\n---\n'
sed -n '1338,1365p' modules/backend/behaviors/RelationController.php
printf '\n---\n'
rg -n "function remove\\(" modules/backend/behaviors/RelationController.php modules -g '*.php'Repository: wintercms/winter
Length of output: 2240
Missing relation constraints let a crafted checked[] payload act on unrelated records. whereIn() is run directly on $this->relationModel, so the request can target IDs outside the current parent relation. Use $this->relationObject with the qualified key and the deferred-binding/parent constraints here; the same pattern also appears in onRelationManageRemove().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/backend/behaviors/RelationController.php` around lines 1246 - 1248,
The relation lookup in the management flow must enforce the current parent
relation instead of querying the base model directly. Update the `$objects`
query near `$foreignKeyName` to use `$this->relationObject`, its qualified key,
and the existing deferred-binding/parent constraints; apply the same correction
in `onRelationManageRemove()`.
2 participants
Problem
Two performance bottlenecks in the core that affect database query counts and bootstrap CPU usage:
N+1 Queries on Relation Deletion:$N$ separate SELECT queries (N+1 queries) before running the deletes, which adds significant network latency when using remote databases (like MySQL on AWS RDS).
When deleting multiple records at once in the backend relation manager, the code queries each record one by one inside a loop. This creates
Topological Sort Overhead on Boot:$O(N^2)$ checks and heavy array-diff operations on every page load.
During bootstrap,
PluginManager::sortByDependencies()runs to sort active plugins. The original logic resolved and normalized dependencies dynamically inside nested loops. On setups with deep dependency chains, this resulted in redundantNote
Even though production sites rarely exceed so many active plugins, optimizing the core sorting algorithm to run in$O(N)$ instead of $O(N^2)$ ensures the system is resilient under complex trees and reduces global CPU utilization under concurrent request loads.
Root Cause
RelationController::onRelationManageDelete:Uses a
foreachloop that calls$this->relationModel->find($id)sequentially for each checked ID instead of querying them in a single batch.PluginManager::sortByDependencies:Runs
$this->getDependencies($plugin)andarray_diff()dynamically on every loop iteration, running redundant formatting and comparisons on static data.Fix
Here are the changes applied to resolve both issues:
1. Query batching in
RelationController.phpModified
onRelationManageDelete()to retrieve all target relation models in a single batch query usingwhereIn(). This preserves model lifecycle events (likedeletinganddeleted) while reducing database round-trips by 49% (100 queries down to 51 for 50 records).2. HashTable lookup in
PluginManager.phparray_diff($depends, $result)call inside the nested loops with a native Zend HashTable lookup (isset($resultKeys[$depend])). This runs inBenchmarks & Results
A. Database Deletion Scale Test (Docker MySQL 8.0)
B. Dependency Sorting worst-case Test (CPU Scale Testing)
Summary by CodeRabbit
Bug Fixes
Performance Improvements