4.2 slight performance degradation, plugin System - WebAuthn requires optimisation

[Fedik]

While profiling Joomla 4.2-dev, I noticed strange popup of json_decode in top, with ~320 calls.

With further investigation, I found that WebAuthn plugin boot up MetadataRepository on plugin boot, and MetadataRepository parse "fido" every time on Boot. https://github.com/joomla/joomla-cms/blob/b219703567293bb305b05cefdbf6df578faa267b/plugins/system/webauthn/services/provider.php#L58-L62

This should be changed, we should not boot anything if it not used at runtime.

Addittionaly, use json_decode(json_encode($entry->metadataStatement), true) should be avoided in MetadataRepository::load. https://github.com/joomla/joomla-cms/blob/f520c98c64efb538e70e918acffb60d07e4d6b1e/plugins/system/webauthn/src/MetadataRepository.php#L193

By looking in the code I do not see a reason of doing it. MetadataStatement::createFromArray can use it as is (if I nothing missed). https://github.com/joomla/joomla-cms/blob/f520c98c64efb538e70e918acffb60d07e4d6b1e/plugins/system/webauthn/src/MetadataRepository.php#L205 If it realy need to be cloned, then should use any kind of "deep clone", but not json or other serialisation (due to metadataStatement structure).

Steps to reproduce the issue

Simpliest way. Visit home page (without login). Open Browser Console => Network tab, and look for response time for a document. Remamber the time. Then disable plugin System - WebAuthn. Go back to home page and check the time.

Advance way: set up xdebug profiling.

Expected result

The time diference should be close to nothing

Actual result

I got on my test installation: ~90-120ms - plugin enabled ~70-90ms - plugin disabled

20-30ms difference, that is a loot of time lose.

beep beep @nikosdion

[nikosdion]

Look, I already said last week in another thread that in my opinion it's a problem that you can only register custom services for Joomla extensions if they are instantiated when the extension boots.

The metadata repository (MDS) is a third party service, defined in the WebAuthn library. I cannot actually make it not load the 300+ root metadata definitions when it's instantiated.

The way the library is written and as far as I can remember I cannot fork it either (either there was a final class or the implementation was so complex that we'd have maintainability issues, can't remember which one).

Here are a few alternatives:

• Tell users to turn off the “Attestation Support” option in the plugin. It's not really necessary, but it's a cop out :(
• Do NOT inject the Metadata Service, instead instantiating it only when needed. That's how I had originally written the code but @laoneo said I should be injecting that service instead.

Out of the two, I prefer the latter. If it's a measurable performance issue then I'd very much like to do the latter. Writing efficient code in real world usage is better than stroking our programming ego (I am a pragmatist, not a code purist).

[nikosdion]

BTW, to make sure that it's a measurable difference we need a slightly different methodology than the one you followed. We need 30 page loads with the plugin enabled and another 30 with the plugin disabled. From that data we need to get the average load time and the standard deviation.

To have a statistically significant page load difference the average page load time with the plugin enabled must be more than the average page load time with the plugin disabled plus 3 times the standard deviation of the plugin disabled sample. If we're within 3 standard deviations we don't have a statistically significant sample; these 0.02 to 0.03 seconds could be down to server performance not being uniform or network jitter.

I am pretty sure that the result will be statistically significant, but I would like to have it in hard numbers to justify the “code impurity” I will inflict upon the plugin 😄

[Fedik]

I understood the reason.

Do NOT inject the Metadata Service, instead instantiating it only when needed. That's how I had originally written the code but @laoneo said I should be injecting that service instead.

Yes, I think that how it should be. We should not boot anything if it not in use, or we will get much bigger perfomance issue. Injecting everythin for a component is probably fine, because it will not be called untill bootComponent. But for plugins it a big NO NO, we need to find a better solition here, somehow. Ideally plugins should be as light as possible.

BTW, to make sure that it's a measurable difference we need a slightly different methodology than the one you followed.

My test instruction in first comment only for a "quick jump" in to the topic :wink: I have used XDebug for profiling, that how I found it. Yeah of course on real and fast server it will be less noticable.

[Fedik]

@laoneo do you have other ideas how to avoid such thing? I mean injectin everything on boot for plugins is a very very bad idea. Plugin may be writen to listen for 1 specific and very rare event, but still will boot some huge librari every time.

Maybe we can inject whole container in to each plugin instance? As Nicholas already suggested somwhere (if I right remamber).

Kind of:

public function register(Container $container)
{
    $container->set(
        PluginInterface::class,
        function (Container $container) {
            $config  = (array) PluginHelper::getPlugin('system', 'webauthn');
            $subject = $container->get(DispatcherInterface::class);

            $container->share(FooBarlib, function() {
                // Do heavy boot
                return new FooBarlib;
            })

            $config['container] = $container;

            $plugin = new BlablaPlugin($subject, $config);
            $plugin->setApplication($container->get('blabla...appl'));
            return $plugin;
        }
    );
}

And in CMSPlugin::__construct:

....
if (isset($config['container'])) {
    $this->setContainer($config['container']);
}
...

In result plugin will boot stuf only when needed, when "event" happen.

[nikosdion]

Yeah, I did suggest injecting the container. Right now it's a service locator. We need a service to be instantiated only right before we use it. That's what service locators are designed for.

If we ever start using the container as a real container we can create a service locator out of it, the same way that Pimple v3, for example, lets you do that (basically, create a subset of the entire container which, ironically, is exactly what we're already doing with the DI containers of Joomla extensions).

Maybe we could just try it in this one plugin for size, you know? Just set up a public setContainer method to pass our DIC / SL from the service provider anonymous class into the plugin object instance when the plugin is instantiated and only use it to get the services we really don't want to initialise on plugin boot. Just like @Fedik suggested. This is what I was talking about in the informal architecture meeting we had over Skype last week. I didn't expect a use case to magically present itself but, hey, beggars can't be choosers here!

The other solution is to create a Singleton factory for that one service and pass that factory to the plugin object but it sounds intrinsically dumb, unnecessarily complex, an anti-pattern and like someone was asleep when people were talking about dependency injection the last, dunno, thirty years or something?

Just thinking out loud. I know how I would do it but, in the end of the day, it's not my decision how to proceed. It's something that needs input from @laoneo @nibra and @HLeithner who are the people where the buck stops when it comes to architecture and who ultimately take the fall if we make an architectural mistake.

[Fedik]

It seems we already can pass container to extension with BootableExtensionInterface https://github.com/joomla/joomla-cms/blob/d05926916287e4a37faa6426a100a23fee2daf1a/libraries/src/Extension/ExtensionManagerTrait.php#L176-L178 And the extension can store $container internally "silently" .

Maybe we can make it more "obvious":

if ($extension instanceof ContainerAwareInterface) {
  $extension->setContainer($container);
}
// and then
if ($extension instanceof BootableExtensionInterface) {
  $extension->boot($container);
}

[nikosdion]

Oh, darn, you are right! I forgot that unlike modules and components, the plugin extension object (which implements the BootableExtensionInterface) is the plugin itself. Doh!

[laoneo]

I do agree that heavy instantiation should not happen in the provider.php file and that we need a proper solution for this. Turning the DI container into a service locator and storing it internally in the plugin sounds on a first glance the easiest way to do, despite abusing the DIC. But something tells me that this will lead to situations we will regret in Joomla land. Can't say right now when. Till now we did it with factories, which allows us to deliver the heavy object when requested. Making for every single object a factory is pretty much overhead, especially when it comes from a library, es the lib should offer that factory and not the core.

I'v avoided to introduce some lazy proxy library where you can proxy any interface you want, as for debugging it would be probably too confusing when a 3rd party extension dev is using print_r($myHeavyObject) and sees some proxy classes instead of an object of the interface. I mean there are already projects offering the functionality to proxy heavy objects and Symfonies DI container can do it out of the box. A good read is this article which describes exactly the situation we have.

As for now I can't really say what would be the proper solution as I'm absorbed with job tasks, but definitely something we must properly solve.

[nikosdion]

So, this plugin is actually causing a lot of very observable slow–down (I checked on my live sites as I have response monitoring stats harvested every minute from 4 separate geographic locations). I think that for now it's best to ab(use) the container and when we have a better solution we can refactor. Plugin classes are final, anyway, so no b/c breaks take place either way.

[laoneo]

Or even instantiate the object which is causing the instance in the plugin and not provider.php.

[laoneo]

I checked on my live sites as I have response monitoring stats harvested every minute from 4 separate geographic locations

We have been discussing this in the german ringcentral channel as well and on some servers it happens and on some not.

[nikosdion]

That was my first idea BUT the problem object (MDS repository) is used to instantiate the Authentication helper object. As a result I will register a service provider for each service and use the container to get the Authenticator helper service exactly when needed.

[Fedik]

Or make a wrapper:

...
$authHelperWrapper = new class() {
  public function getAuthenticator() {
     .... blabla code
     return new Authentication($app, $session, $credentialsRepository, $metadataRepository);
  }
}
$plugin = new Webauthn($subject, $config, $authHelperWrapper);
...

well, probably also need inject container in to it also :)

[nikosdion]

I prefer using services. The idea is that a third party plugin MAY override the MDS service in the global container. If one is already set we don't have to set our custom service in provider.php.

[laoneo]

As a temporary workaround I suggest to do it like in #3866. Can you guys have a look if I did something wrong.

[brianteeman]

just wanted to point out that on my localhost server the degradation is significant

[nikosdion]

@brianteeman Does the file administrator/cache/fido.jwt get created and non-empty?

Also, can you check if it changes its last modified date/time every time you reload a page on the site?

[Fedik]

The idea is that a third party plugin MAY override the MDS service in the global container.

Can we pass callback in to constructor?

...
$authCallback = function () use ($container) {
   .... blabla code
   return new Authentication($app, $session, $credentialsRepository, $metadataRepository);
}
$plugin = new Webauthn($subject, $config, $authCallback);
...

Kind of JavaScript style :)

[brianteeman]

Same page - with and without the plugin

I checked the file as requested and it was NOT empty.

I deleted the file and refreshed the page and the file was recreated but is now empty and the site performance is back to good.

the file is not being updated on refresh now (well its still 0 bytes and the timestamp didnt change)

Hope that helps

[weeblr]

@nikosdion If the Fido alliance server is down (which it seems to be this morning), there's an error in the fido.jwt caching logic that causes the plugin to retry connecting to the server on each page load, hence why we've got a number of people with 5 seconds load time on their J4 site today.

See https://github.com/joomla/joomla-cms/issues/38662

[nikosdion]

@weeblr Yup, I am noticing the same.

The solution I am working on is to download this file when building Joomla, decode it and store a serialised copy of the decoded data with the plugin. This way we can avoid both the download issue and the slowdown from decoding the JWT token.

[brianteeman]

its a perfect storm

[weeblr]

@nikosdion Makes sense. If that can be implemented quickly, it's better. In the mean time, need to get the word out about disabling the webauthn plugin I guess? @brianteeman

[Fedik]

need to get the word out about disabling the webauthn plugin I guess?

Enough turn off the “Attestation Support” option in the plugin

[weeblr]

@Fedik I think "Disable this plugin" is simpler. Not sure if we have usage stats about webauthn but 100% of the users on the French J! user group had no idea what it was at all. Disabling should be fine I think.

[richard67]

Closing as having a pull request. Please test #38664 . Thanks in advance.

[yackermann]

Oh boy. Thanks for fixing it. You guys done 120gb of traffic over the weekends. Next time let me know, I am happy to help you with deployments and optimisation *)

[HLeithner]

@herrjemand sorry for the inconvenience, I hope it doesn't created too many issues.

[brianteeman]

@herrjemand What was the site?

[richard67]

@brianteeman It's the Fido site.

[brianteeman]

ah - cool

[yackermann]

it's fine.

Let me know how I can help in future. *)