Lower the impact of EA on non-admin requests

[tucksaun]

Hi!

I think I found what can be a nice performance improvement to reduce the impact of EasyAdmin when not in actual use (requests that are not served using an admin controller), in particular for API requests that do not use Twig.

Currently, the AdminRouterSubscriber forces Twig initialization to inject the ea variable, even if Twig is not used (API endpoint for example). Also, the CrudControllerRegistry causes the load and initialization of every single admin class, plus this registry and DashboardControllerRegistry are doing array computation in their constructors, but because of the way things are wired and that AdminRouterSubscriber listens to kernel.request, this happens on every single request served by the Symfony app.

With this patch, I managed to bring a fast API endpoint from about 30 ms down to about 18 ms, and from 7.69 MB of memory down to 4.84 MB. :warning: Disclaimer: those measures have been done against EA3.

It is important to note that not every project would get exactly the same performance boost as it highly depends on how much time is required to initialize Twig. Though I believe it is nice to allow projects to reach this level of performance when needed. Also, even a couple of milliseconds and hundreds of kB of memory won by not loading the admin controllers and dashboards is still a win for everyone :)

[javiereguiluz]

This is brilliant: pre-computing the controller FQCN cache and injecting the Twig variable looks so obvious now that you submitted your contribution! I wish we had done this earlier. Thanks a lot for taking care of improving performance in this way.

I've tested this in a real complex Symfony + EA app. It works great in all backend pages, except for the dashboard index page (/admin) where I see the infamous "Variable "ea" does not exist." error. I need to check if it's something related to my custom code or some condition that it's triggering some error somewhere. Thanks!

[javiereguiluz]

I know the cause of the mentioned issue. In my backend dashboard, I do a $this->render(), which creates a Response object. When a controller returns a Response, the kernel.view event is not triggered.

I could solve this by listening to kernel.request event with a priority lower than 0 (because EA's AdminRouterSubscriber::onKernelRequest() runs on 0 and creates the context variable). We also need to inject a nullable twig service and return early if twig service is null.

Would this change work for you ... or would that nullify the improvements you gained with this change? Thanks!

[tucksaun]

@javiereguiluz thank you for looking into this. I forgot about this use case 🤦 your suggestion would indeed nullify the improvement but let me see what I can do about this.

[tucksaun]

@javiereguiluz I followed @stof advice and went for the Twig extension implementation. As there is already a Twig extension present, I reused it. But I changed the constructor signature so let me know if you believe this is too much of a BC break and want me to adapt it.

[maxhelias]

The only issue I can see is if one renders a Twig template before kernel.request is triggered and then Symfony goes on with request processing. But as you said previous code would then have failed too 🤷

Is this isuue still blocking? What about moving $this->adminContextFactory->create on the adminContextProvider and only call him when we don't have the context ?

[tucksaun]

@maxhelias good question! I guess this is up to @javiereguiluz to determine if this is a blocker or not. In the meantime: PR rebased

[tourze]

Make the twig service lazy, can reduce the problem. like:

  App\Restful\Twig\Environment:
    decorates: twig
    parent: twig
    lazy: true

[javiereguiluz]

I apologize for not having merged this much earlier. I've tested this again and all my apps worked as before, so let's merge this one to get that nice performance improvement for non-admin requests.

@tucksaun thanks a lot for such a nice contribution! And thanks to reviewers too!

[Qronicle]

FYI: This breaks the global ea twig variable when you use twig before the context is initialized.

Eg. if I put something like this in my DashboardController implementation:

    public function configureAssets(): Assets
    {
        return parent::configureAssets()
            ->addWebpackEncoreEntry('app')
            ->addHtmlContentToBody($this->renderSomeTwigTemplate());
    }

This will initialize twig without ea variable, because I assume that when configureAssets is called, the context is not yet initialized.

(We solved this by just overriding the template instead of adding the HTML here)

[javiereguiluz]

@Qronicle thanks for reporting this issue. I'd say it's fine to not fix this for now.

First, the addHtmlContentToBody() is already an edge feature not commonly used. Second, it's mostly designed to add some small HTML snippet (think of some tracker or some custom and legacy HTML content needed by the company). In the case of rendering a full template with the contents, it's definitely not designed to render an EasyAdmin template or something that needs EasyAdmin features. For those edge cases, as you perfectly said, the ideal solution is to override the EasyAdmin templates.

If we get more reports from users, we'll reconsider this and we'll try to fix it. Thanks.

[Qronicle]

it's definitely not designed to render an EasyAdmin template or something that needs EasyAdmin features

I doesn't have to be, the example you gave is more or less what we used it for, completely separate from anything EasyAdmin related.

But instead of writing the HTML in the controller, we rendered it using twig. This initializes the global twig environment variables, and because the EasyAdmin context is not yet known at that point, the ea global will never be available.

Edit for clarification: The issue is not that we cannot use the ea global in the template we render in the addHtmlContentToBody function. The issue is that the ea global is not available when, later in the request, the actual EasyAdmin dashboard-powered controller tries to render the EasyAdmin templates. (Eg. the default crud index action template)

[javiereguiluz]

Just wondering: could we use Twig extension priority (https://symfony.com/blog/new-in-symfony-4-1-twig-extensions-priority) to solve this problem by running our extension as soon as possible?

[Qronicle]

I assume that is just about the order twig loads extensions in, and will not help with the goal of this MR in mind.

(If I understood the performance gain of this MR; it is that we don't want to initialize the twig environment whenever we know the EA context, because we might never need twig anyway, so why initialize it, right?)

I was thinking up a solution, but then I noticed this exact risk was already mentioned in the review of this MR. (Sorry!) My working solution would probably also involve the ea global being null by default, but @tucksaun already ruled this out:

As an alternative, my very first attempt at this was to inject a "dynamic" global variable that would retrieve the right context at runtime but it broke the fact that EA templates rely on ea being defined or not versus null or not.

So yeah, not immediately sure what you could do to solve this either. To solve our case specifically, I might take a look at why the context is not yet available in the configureAssets method. But that'll have to wait for next week.