Inspired by the comment #2429617-36: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) by Wim Leers I had working on profiling/optimization of two functions

\Drupal\Core\Asset\AssetResolver::getCssAsset()
\Drupal\Core\Asset\AssetResolver::getJsAsset()

Sub-optimal performance of these 2 functions originated by reason descried there: #2381473: Decrease the size of the asset library cache item

However, I propose a simple patch which:
- does some minor optimization by moving array declaration out of the loop;
- removing usage of an obsoleted Drupal function;
- imposes unified array-declaring syntax withing those functions instead of mixing array and []

CommentFileSizeAuthor
#1 Tuning_of_the_AssetResolver_class_2470833_1.patch3.86 KBdpovshed

Comments

dpovshed’s picture

dpovshed commented
Status: Active » Needs review
StatusFileSize
new Tuning_of_the_AssetResolver_class_2470833_1.patch3.86 KB

The patch attached

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -optimization +Performance, +Quickfix

Nothing to remark. Perfect. Thank you! :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +D8 Accelerate Dev Days
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
@@ -130,7 +131,7 @@ public function getCssAssets(AttachedAssetsInterface $assets, $optimize) {
+                $options['basename'] = \Drupal::service('file_system')->basename($options['data']);

Let's inject this.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

#4: heh… that's exactly what I considered pushing back this issue for, but… this will likely go away with #2389735, see #2389735-124: Core and base theme CSS files in libraries override theme CSS files with the same name. Hence it didn't seem like it was so bad if this is using \Drupal::service() temporarily, it'd avoid us from having to add a lot of boilerplate here, only to remove it later.

dpovshed’s picture

dpovshed commented

Thank you guys for your feedback!

BTW while I look for replacement for function drupal_basename(), I checked 2 places:
1) source of the function going to be obsoleted;
2) changerecord for the function here https://www.drupal.org/node/2418133

If we expecting more optimal replacement from core and module contributors, I think it may have sense to update change records. Thanks!

  • alexpott committed 8bb0609 on 8.0.x 
    Issue #2470833 by dpovshed: Tuning of the AssetResolver class
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

@Wim Leers okay - let's not handle the injection here - I agree that adding it to remove it later does not make sense.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 8bb0609 and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.