Performance improvements - Limiting the number of classes loaded by the container

[jorgemd24]

User story

This is a follow-up from #1244(comment) and the issue #1205.

Is your feature request related to a problem?

After the investigation of #1205, we detected that we could limit the number of classes loaded by the container on every request. In PR #1244 we did an improvement related to the loading of the NoteInitializer and the GoogleAdsClient classes and we saw a significant improvement in the performance.

We could continue doing an investigation to limit the number of classes loaded by the container depending on the request. For example, it seems that for some fronted requests, it would not be necessary to call the method maybe_register_services and consequently improve the performance. There are other cases where the classes are only needed for scheduled jobs, we could think to load these classes only when the scheduled jobs are executed.

How to reproduce the problem

Debugging the function $services = $this->container->get( Service::class ); and checking the number of services loaded and the time that it takes to finalize that call.

Describe the solution you'd like

In the function where we are registering the classes, i.e maybe_register_services, we could load the services depending on the type of the request:

• Scheduled_Services -> $services = $this->container->get( Scheduled_Services::class );
• Rest_Services -> $services = $this->container->get( Rest_Services::class );
• Guest_Services -> $services = $this->container->get( Guest_Services::class );

In this way, we have more control of the classes that are loaded.

Technical

We will need to do a deeper investigation of how we can split these services and how we could limit the number of classes loaded depending on the type of request.

[mikkamp]

This issue has recently surfaced again with the following profiling: Note: this is taken from a site where GLA hasn't even been setup/onboarded yet

I think it's worth investigating again what's actually happening in the call to initialize GoogleAdsFailure: https://github.com/woocommerce/google-listings-and-ads/blob/2.5.5/src/Google/Ads/GoogleAdsClient.php#L36 Especially since we have our own error handler. We could also consider calling it just before an actual API request so it doesn't need to run on construction.

[jorgemd24]

@mikkamp, I investigated deeper into why GoogleAdsFailure:init consumes more resources than expected.

The Problem:

The primary issue arises when the init function is executed, which attempts to parse two files. The more resource-intensive of the two is this one: https://github.com/googleads/google-ads-php/blob/main/metadata/Google/Ads/GoogleAds/V14/Errors/Errors.php. This leads to the function calling other functions, such as parseFieldFromStream, more than 11,000 times.

Initiating GoogleAdsFailure:init is known to be a resource-intensive process, as you can see here: googleads/google-ads-php#231. That's why they implemented lazy loading for the gRPC protocol here: googleads/google-ads-php#493. Essentially, they added an interceptor to the call so that GoogleAdsFailure:init is only invoked when there is an error.

The reason why GoogleAdsFailure:init was called in the constructor can be traced back to this issue: googleads/google-ads-php#212. The goal was to resolve the following error: Class google.ads.googleads.v2.errors.GoogleAdsFailure hasn't been added to the descriptor pool when a partial job or batch job contains errors. The HTTP response status will be 200 but the response could contain errors.

Currently, the Ads client in the GL&A extension is created using the GoogleServiceProvider, which is invoked every time an admin screen is loaded or any request to wp-json is made. This is why it impacts overall performance.

Potential Solution:

Considering that we already have an error_handler middleware in place, we could initiate GoogleAdsFailure:init only if the request is made specifically to google-ads, rather than invoking GoogleAdsFailure:init in the GoogleAdsClient: constructor and for every single call.

I couldn't find any requests in GL&A that use partial failure, and simply disabling GoogleAdsFailure:init should work as expected. This is because errors are processed using UnaryGoogleAdsExceptionMiddleware, which will eventually trigger the initiation of GoogleAdsFailure when necessary.

I did a simulation of a partial failure changing one of the requests to use partialFailure=true and not calling GoogleAdsFailure:init. When the API responded with an error, I received the following error: Class google.ads.googleads.v2.errors.GoogleAdsFailure hasn't been added to the descriptor pool. However, when I repeated the same steps but called GoogleAdsFailure:init in the error_handler middleware, I received the expected errors.

In my view, including GoogleAdsFailure:init within the error_handler middleware should be enough to enhance overall performance. If we are happy with this, I can go ahead and create a PR with the solution mentioned above.

[mikkamp]

Thanks for the thorough investigation, adding it to the error_handler sounds like a great way to resolve the issue.

Do you have a "before" and "after" comparison on resource usage, to help determine if the rest of this issue (selective service loading) still makes sense to leave open?

[jorgemd24]

Here are the cachegrind reports:

Before:

After:

Shifting GoogleAdsFailure:init to the error_handler will help in cutting down the time allocated for the Google protobufs functions. However, the GL&A container is still consuming roughly 4.88% of the total loading process, which could indicate room for potential improvement.