If you look at the following services definitions you will see that there are quite of them which don't need to be public:
asset.css.collection_optimizer:
class: Drupal\Core\Asset\CssCollectionOptimizer
arguments: [ '@asset.css.collection_grouper', '@asset.css.optimizer', '@asset.css.dumper', '@state' ]
asset.css.optimizer:
class: Drupal\Core\Asset\CssOptimizer
asset.css.collection_grouper:
class: Drupal\Core\Asset\CssCollectionGrouper
asset.css.dumper:
class: Drupal\Core\Asset\AssetDumper
asset.js.collection_renderer:
class: Drupal\Core\Asset\JsCollectionRenderer
arguments: [ '@state' ]
asset.js.collection_optimizer:
class: Drupal\Core\Asset\JsCollectionOptimizer
arguments: [ '@asset.js.collection_grouper', '@asset.js.optimizer', '@asset.js.dumper', '@state' ]
asset.js.optimizer:
class: Drupal\Core\Asset\JsOptimizer
asset.js.collection_grouper:
class: Drupal\Core\Asset\JsCollectionGrouper
asset.js.dumper:
class: Drupal\Core\Asset\AssetDumper
library.discovery:
class: Drupal\Core\Asset\LibraryDiscovery
arguments: ['@library.discovery.collector']The following could be marked as public: false
Mark them as public: falseand let the testbot decide whether this is okay.
| Issue category | Task, because there is no real problem |
|---|---|
| Issue priority | Normal because the impact is not high |
| Prioritized changes | The main goal of this issue is to improve performance. |
| Comment | File | Size | Author |
|---|---|---|---|
| #34 | 2354705-34.patch | 1.89 KB | narendra.rajwar27 |
| #31 | 2354705-31.patch | 1.89 KB | narendra.rajwar27 |
| #4 | Mark-a-couple-of-asset-services-as-non-public-2354705-2.patch | 1.63 KB | yannisc |
| #1 | Mark-a-couple-of-asset-services-as-non-public-2354705.patch | 1.59 KB | yannisc |
Comments
Comment #1
yannisc commentedYou mean something like that?
Comment #2
dawehnerAwesome work!
Comment #4
yannisc commentedThanks, dawehner!
I updated the patch, as the original one does not apply any more.
Comment #5
Crell commentedAnd back.
Comment #6
wim leersAFAICT this does not have negative consequences for contrib: they can still override all of the services, including those marked as private.
It does look like it might make alternative asset processing pipeline implementations, that use these services in a different way a bit more difficult, but AFAIK it's still possible to make private services public again by having a container rebuild event listener.
Finally, it's easy to make private services public if we want to do that later; that'd be an API addition, the reverse direction is an API break.
Assuming I've made no mistakes in the above: +1 :)
Comment #7
alexpottCommitted af83168 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation for to the issue summary.
Comment #9
webchickOver at #2378737: Consider not executing hook_library_(info)_alter() *per* library, but once for all this issue is cited as causing a 150ms slowdown in testbot runs. Adding as a related issue for now, but basically I think we should just roll this back.
Comment #10
wim leersThat is actually due to a Symfony bug, this patch didn't do anything wrong.
Comment #11
alexpottI agree with @webchick reverted this. We can still do this because having unnecessary public services is a performance hit. But we need to ensure that doing this does not introduce a far bigger perf regression. According to @dawehner the problem is caused by https://github.com/symfony/symfony/issues/12924.
Comment #13
alexpottre #10 I agree but lets get the test performance benefits and ensure that a patch that is supposed to deliver those benefits does - which means we need to do fix upstream first.
Comment #14
wim leersFair enough :)
Comment #15
wheatpenny commentedI am removing the Novice Tag from this issue as there are no remaining tasks.
Comment #29
cyb_tachyon commentedNoting here that https://github.com/symfony/symfony/issues/12924 was closed soon after it was opened as "Won't Fix / Not a Bug".
Comment #30
narendra.rajwar27Comment #31
narendra.rajwar27Patch from comment #4, re-rolled and updated for 8.9.x branch.
Comment #32
kristen polIf this is still relevant, it needs a reroll for 9.1.x.
Comment #33
alexpottWe still need. to look into the performance regression this caused originally. In Symfony world public services are very rare in fact we have to force our container to make services public by default.
Comment #34
narendra.rajwar27Patch Re-roll for 9.1.x.
Comment #35
narendra.rajwar27un-assigning the issue. Please review.
Comment #36
alexpott@narendra.rajwar27 if you want this issue to progress then you need to work on #9 - ie. the performance issue caused when we committed the patch.
Comment #37
kristen polMoving back to needs work per #36.