Problem/Motivation

The AMP service initialization is extremely slow, there is a ton of stuff happening in the lullabot library just when constructing it.

Would be nice to look into improving that too, but actually what I noticed is that it gets called on every request, not just AMP.

Proposed resolution

Move the initilization in AmpHtmlResponseMarkupProcessor (which is invoked on every request) from the constructor to the method where it is needed.

Another approach would be to make the whole service definition lazy because AmpHtmlResponseSubscriber only calls it on amp pages. But even then, the setting might be disabled, so that should then be moved out to the response subscriber.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
amp-constructor-lazy-initilization.patch1.04 KBberdir

Comments

Berdir created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/src/Render/AmpHtmlResponseMarkupProcessor.php
@@ -111,6 +110,8 @@ class AmpHtmlResponseMarkupProcessor {
 
+    $this->ampConverter = $this->ampService->createAMPConverter();
+

Can we ensure to just initialize it once? This method (processMarkupToAmp) might be called multiple times.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

As we discussed, the constructor of that object has its own singleton, so not really that important. And other places in the code call it too, so I think if we want to ensure a single object, then in that method.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

This is certainly a valid bugfix!

  • mtift committed 45f3c47 on 8.x-1.x authored by dawehner 
    Issue #2828464 by Berdir, dawehner: Only initialize AMP service when...
mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
commented
Status: Reviewed & tested by the community » Fixed

Good catch! Committed and pushed to 8.x-1.x. Thanks, guys.

Status: Fixed » Closed (fixed)

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