Discussion - Extension Performance

[fooman]

Performance comes up at various times in regards to extension quality. I am not sure if there is much that can/should be done as part of a codesniffer rule as load testing might be better, for example with the Magento performance profiles, but adding a topic here so we don't lose the discussion https://twitter.com/foomanNZ/status/1018553321555021824

[jissereitsma]

Noted. I'm not sure if this is easy to do with PHPCS, but perhaps the new Reflection helper I've added comes to aid. Few other things come to my mind: Blackfire performance testing in a CI chain. And perhaps an Integration Test that is able to sum up the queries that have been added since a specific piece of code has been run. Both would be optional for developers, but at least more thought can be put into this.

[schmengler]

Specific performance related anti patterns (like load in a loop) can be implemented as code sniffer rules, but will be bypassed if they are hidden behind abstractions as described in the Twitter thread.

But let's keep this topic to collect ideas independent of phpcs

[dankoz51]

I have had to fix many extensions that broke the caches which is a huge performance hit. Test automation can detect cache headers and the Magento debug cache header to verify the extensions don’t break the full page cache

I have also had one invalidate the config cache and that was a nightmare and hard to find.

[jissereitsma]

@dankoz51 Good point. Magento Marketplace already contains a check to see if FPC is disabled once a tested extension is enabled. Likewise they are working on performance metrics to see if an extension puts an additional load somewhere. To built a FPC check is not that hard I think, it could be done with an integration test in a controller that dispatches the request and then inspects the response.

Could you elaborate a bit more on the extension that invalidated the config cache? Was this on every request? Or specific circumstances?

[dankoz51]

So far three different issues.

1. Block layout specifies cacheable="false" and that breaks FPC on the entire page, where as the developer is thinking it disables for the block.
   Perhaps a warning on this would be good, more details here.

2. The built in (not varnish) full page cache was broken by a major theme vendor/magento partner. They were writing to the http context to track some user info that would cause the vary key to change, and therefore change the page hash. So like this

   1. request comes in, magento evaluates it and gets the page hash (url/vary seeds) lest say its 123
   2. magento looks for 123 in internal FPC , does not find it
      3.extenstion modifies the http context (cookie i think)
   3. magento then recalculates the page hash (cookie/new vary key now that http context changed) lets say it's 456
   4. magento saves the page in fpc with key 456
   5. next request the hash is calculated as 123 and no cache hit, as the page was saved with 456
      The point here is that FPC internal and FPC external are both different and may require different tests.

3. Config cache issue was on every request that was not cached. So it was on http ajax requests for the cart etc. I think they were trying to invalidate the cache correctly on admin save, but something resulted in it doing it all the time. The FPC sort of masked the issue for us as cached pages loaded fast, but if it was not in the cache we saw about an extra 3 seconds.

[jissereitsma]

Thanks for the input! Much appreciated. To comment on the points:

1. I know Magento EQP has a tool already in place to check for this. It is definitely something that could be part of this ExtDN PHPCS project. However, this requires a PHP CodeSniffer handler for XML files and strangely, that doesn't exist yet. Which makes you wonder if PHPCS is a correct tool for analysing XML files. Other tools might be used as well. And actually, I like another approach of Magento EQP even better: They check whether vital pages still support FPC when validating an extension.

2. This is a tough one. In custom solutions, I've seen changes like this done on purpose (changing the hash that generates the Vary header) which again required changes on the Varnish side. Custom cookies also require the same thing normally. However, it would not be the responsibility of a 3rd party module to change things like this. I'm not sure if this is easily detected.

3. This is again something that Magento EQP has been working on: Benchmarking pages to see if a certain page loads differently as soon as an extension is installed. You could also build the same thing with the commercial (?) Blackfire tools (performance testing). However, the main reason I was asking is to see if this could be something that leads to tools that could be easily run on the extension code (not real-life testing, not necessarily integration or functional testing) to see if this pops up. Theoretically, this would lead to a sniffer rule that sees whether a cache is invalidated (cache->invalidate) but I'm afraid it will be hard to prevent false positives (there might be a really good reason to invalidate the cache) or too few true negatives (there might be other ways to mess things up).

Last but not least, did you stick around for the vendors to fix their extensions? Or did you chuck them out?