[WIP] perf(scd): skip redundant per-file deploy for inherited locale/theme variants via bulk copy#40619
[WIP] perf(scd): skip redundant per-file deploy for inherited locale/theme variants via bulk copy#40619SamJUK wants to merge 2 commits intomagento:2.4-developfrom
Conversation
…mpilation When deploying a locale/theme variant whose files are all (or mostly) inherited from a parent package, copying the already-deployed parent directory is orders of magnitude faster than re-running the LESS pipeline for each file individually. Changes: - DeployPackage::deployEmulated() checks shouldBulkCopy() before the per-file loop; if the threshold is met it calls DeployStaticFile::copyTree() to copy the entire parent tree in one pass, then only processes non-inherited files individually. - shouldBulkCopy(): fires when >= BULK_COPY_THRESHOLD (0.5) of files are inherited from the parent package. The 0.5 threshold ensures we only skip the per-file loop when the bulk of the work would be redundant. - isInheritedFile(): O(1) hash lookup - isset($parentPackage->getFiles()[$fileId]) - copyTree(): uses pubStaticDir->copyFile() / pubStaticDir->create() rather than native copy()/mkdir(). Benchmarking shows the abstraction is ~2.5x faster per file (0.038ms vs 0.1ms) due to @copy() suppression in the File driver. - Fixed checkIfCanCopy(): was comparing origPackage (deepest ancestor virtual package) to the direct locale parent; virtually never matched. Now compares getPackage(). - QuickDeploy: pre-compute $queuedPaths and only pass a parent as a queue dependency when that parent is actually in the queue. Prevents deadlocks when --no-parent or --theme filters exclude theme-hierarchy parents from the deploy set. - Extracted magic number 0.5 to DeployPackage::BULK_COPY_THRESHOLD constant. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Applies the same bulk-copy optimisation to the standard deploy strategy that commit 4da59cb introduced for the quick strategy. Changes: - StandardDeploy::deploy() identifies base locale packages per theme and assigns them as parents for variant locale packages in the same theme. - Locale packages are sorted so base packages (no parent) are queued first, ensuring the parent directory exists before any variant tries to copy it. Uses usort() in place of the previous double array_filter + array_merge. - Each variant package is queued with its parent as a dependency, so parallel (-j N) workers cannot start copying before the parent deploy completes. Benchmark results (3 stores, quick strategy shown for comparison): Store 1 (4 themes, 2.4.8-p4): standard 64% faster, quick 75% faster Store 2 (7 themes, 2.4.8-p3): standard 69% faster, quick 81% faster Store 3 (11 themes, 2.4.8-p4): standard 73% faster, quick 89% faster Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Hi @SamJUK. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
|
The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email psirt@adobe.com. |
SamJUK
changed the title
|
The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email psirt@adobe.com. |
1 similar comment
|
The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email psirt@adobe.com. |
|
@magento run all tests |
1 participant
Description (*)
When deploying locale/theme variant packages, the vast majority of files are inherited unchanged from a parent package. Currently both the quick and standard strategies re-run the full LESS pipeline for every file in every variant, even though LESS output does not vary by locale and the files are identical to the parent.
This PR replaces per-file deploy operations with a single recursive directory copy for packages whose files are predominantly inherited, across both the quick and standard deploy strategies.
Performance results (5 locales: en_GB en_US fr_FR de_DE nl_NL, cold static dir, serial mode):
The compact strategy is unaffected (~2% noise) — it pre-compiles LESS once then copies cheaply, so
shouldBulkCopy()correctly returns false.Breakdown
Root cause - quick strategy (dead code):
checkIfCanCopy()contained the condition$file->getOrigPackage() === $parentPackage.origPackageis set to the deepest ancestor virtual package (e.g.blank/default), not the locale parent assigned by the strategy. This condition virtually never passed, so every file fell through todeployFile()regardless of whether a copy was valid.Root cause - standard strategy:
The standard strategy had no parent-package concept at all. Every locale of a theme was deployed fully independently, re-running LESS compilation for each.
Parallel safety:
copyTree()requires the parent locale directory to be fully deployed before a variant worker begins copying. Without explicit queue dependencies,-j Nworkers can fork a variant before the base locale finishes. Both strategies now pass the parent as an explicit queue dependency.Deadlock fix (quick strategy):
QuickDeploy::preparePackages()assignssetParent()for both locale parents and theme-hierarchy parents (e.g. blank as parent of luma). When--no-parentor--themefilters exclude theme-hierarchy parents from the package pool, passing them as queue dependencies causesQueue::process()to wait indefinitely. The fix pre-computes$queuedPathsand only uses parents present in that set as dependencies.Manual testing scenarios (*)
Basic deploy (all strategies):
pub/static/:rm -rf pub/static/frontend pub/static/adminhtml pub/static/basephp bin/magento setup:static-content:deploy -f en_GB en_US fr_FR -s quick-s standardParallel deploy:
pub/static/php bin/magento setup:static-content:deploy -f en_GB en_US fr_FR -s quick -j 4-s standard -j 4Filter options (deadlock regression):
php bin/magento setup:static-content:deploy -f en_GB en_US --theme Magento/luma --no-parent -s quickOutput correctness:
find pub/static -type f | sort > /tmp/stock.txtfind pub/static -type f | sort > /tmp/patched.txtdiff -r /tmp/stock.txt /tmp/patched.txtproduces no outputContribution checklist (*)