Optimize AppStorage::loadUnchanged

[vitalie-cracan]

Apigee can be quite slow. Reducing the number of requests would help. I noticed that AppStorage::loadUnchanged($id) is making two identical requests. It does it because it needs to cover the possibility that $id can sometimes be an app name and not an app uuid.

I am new to the module and I have actually never seen a case when $id is an app name. Is this still a possible scenario?

Could we maybe pattern match the id and if it fits a uuid pattern then simply remove that id from cache? And if it does not fit the uuid pattern, only then have it load the app first to get the app id?

https://github.com/apigee/apigee-edge-drupal/blob/8.x-1.x/src/Entity/Storage/AppStorage.php#L92

[mxr576]

Apigee can be quite slow.

This is the reason why this multi-layered, extensive cache system have been invented to minimize the communication with the API backend.

I have actually never seen a case when $id is an app name.

An app could be loaded several different ways:

• by UUID
• by owner id (developer email, company/team name) + app name
• by owner uuid (developer uuid) + app name

These are the scenarios that we have to support, so technically yes, it is possible that an app is loaded by its name if the owner is in the context.

Uncached load - in general - should be minimized, even if the storage is not Apigee. Is it a custom code where are you facing with performance issues or an OOTB UI provided by this module?

[vitalie-cracan]

Thanks for your comment Dezső. The scenario I was referring to is reproducible on a clean standard Drupal install with only apigee_edge module enabled. One can see it when when one goes for the second, third, etc time to the edit page of an App, /user/{id}/apps/{app_name}/edit.

I've been a bit more careful now that you asked to describe how this happens and I see that after a Drupal cache clear, the callstack is slightly different, but the bottom line is that:

DeveloperAppNameConverter::convert() always queries Apigee twice for a given app. When cache was already warmed by:

https://github.com/apigee/apigee-edge-drupal/blob/8.x-1.x/src/ParamConverter/DeveloperAppNameConverter.php#L81

The two calls are exactly same and happening in AppStorage::loadUnchanged, since

https://github.com/apigee/apigee-edge-drupal/blob/8.x-1.x/src/ParamConverter/DeveloperAppNameConverter.php#L90

passes it a uuid so loadUnchanged first unneccessarily makes a call to Apigee to make sure it has the uuid, then clears cache, then calls to the Apigee with exactly same request in parent::loadUnchanged($id).

If cache is not warm, there is a call for the App to Apigee by app_name, the uuid is then cheaply retrieved from cache, cache is deleted and parent::loadUnchanged($id) again calls Apigee for the same app, this time by uuid.

All these happen in the call to DeveloperAppNameConverter::convert(). Whatever you do, you get two requests to Apigee for same app. I see no need for this to happen, just do not know how to fix it. Of course, an app can be named with a uuid, because someone can.

It looks like loadUnchanged wanted to be too generic at the cost of efficiency and the right solution would be to have different functions for when a name is passed and when an uuid is passed.

Any thoughts?

Thanks, Vitalie

[mxr576]

Entity edit forms in Drupal always have to load the latest version of the data from the storage, they cannot rely on cached data. Especially in the case of Apigee, when the storage itself could be modified in multiples places, not just in Drupal.

It looks like loadUnchanged wanted to be too generic at the cost of efficiency and the right solution would be to have different functions for when a name is passed and when an uuid is passed.

Probably you are right, as I described above, this method has to support all possible "load app" scenarios. Although, additional utility methods could be introduced in the storage to load an unchanged app by name, for example, loadUnchangedByName().

I am not entirely sure if that would help to resolve this issue - now it kinda rings a bell - but this only occurs on app-related pages where the unchanged version of the app has to be loaded. Correct me if I am wrong, but currently, this is only the edit form, maybe the delete form too. View and listing pages are always leveraging entity storage cache (besides render cache).

The name converter is also a must-have, otherwise, pretty URL-s for apps would not work. If you do not need those, you can also build custom UIs that are relying on UUIDs in paths instead of owner id + app name.

[vitalie-cracan]

Yes, so far I have noticed this only for the edit and delete forms. I have created another pull request:

https://github.com/apigee/apigee-edge-drupal/pull/460