Defining the Drupal 8 performance gate

Lots of patches have minimal performance impact. Lets let them say N/A and move on with life.

This is true, but we need criteria for what that means. http://drupal.org/node/1203766, http://drupal.org/node/1061924, http://drupal.org/node/1014130 and http://drupal.org/node/1064212 are all issues documenting quite serious regressions in core that resulted from 'innocent' patches in the first place. On only one of the issues that introduced those regressions did anyone review the code in such a way that these issues would be caught (and I reviewed at least two of the original regression-causing patches and asked for benchmarks in one). I think we need to shift at least part of the burden towards those involved in an issue to indicate why it doesn't cause a regression - I'm fed up with coming across them months after issues are committed, not all are avoidable but a lot are.

It is easy to increase performance by adding caches. But each cache cuts DX a little and we can easily get into a situation where nobody knows WTF is going on and devs start throwing cache_clear_all() and menu_rebuild() and so on all over. Please lets keep code sane and only cache when we really need it.

This is related to the memory section about adding new static caches (or changing the content of static caches), if we can increase to 6 items then this is worth adding I think. While I'm in favour of adding caching where we should, I've also seen issues try to add pre-emptive persistent caching for things that only call a few cheap PHP functions in core, just in case a contrib module does something expensive - which is adding both DX and actual performance overhead to core (hitting the cache layer isn't free either) for hypotheticals. I've also seen cases where we had marginal static caches, that were later changed from static to drupal_static(), and the drupal_static() was as expensive or worse than the code that was being static cached in the first place.

There's some bits of documentation we'll need for this:

• a section in the handbook on performance testing and profiling tools, basic install howtos, how to use them, how to share results in the issue queue.

• a handbook page with example code for microbenchmarks.

• maybe a new 'performance' section of coding standards, to try to document patterns where we have them.

@mikeyP@mikeytown2 - I added a note about variable_set() to the docs. We don't have documented patterns for using the locking framework in core, there are several core issues outstanding on that, so we'd need to resolve that and come up with a repeatable pattern before it can be added here IMO, although a section on race conditions and stampedes might well be worthwhile.

@rfay see here:
http://drupal.org/sandbox/catch/1186744

There's already discussions in that queue.

Simpletest isn't run from Jenkins for qa.drupal.org, it's run on PIFR which is a Drupal-based test runner in PHP.

When we have specific automated performance tests to run on branches, it's very likely these will be Jenkins jobs - but that framework doesn't exist yet. However to deal with potentially differences in hardware for test runs, we've come up with a design that does not rely on wall time but rather measures resources used, you can read more about that at http://groups.drupal.org/node/133679

What qualifies as "large" objects/arrays? What qualifies as "lots" of PHP files/includes?

There is no easy answer to this, especially between full bootstrap and page caching, that's part of why page caching broke so many times and in so many different ways during D7 - because people benchmarked full page requests to validate changes to code that runs during cached page requests. Also the size of objects and arrays can vary massively between sites in a lot of cases.

"lots" of PHP files/includes could probably be "more than a couple" - since usually you are changing one or many but not often in between.

"CPU" still needs work, as well. Most Drupal developers have not stepped through index.php in a debugger, so have no idea what "especially up to and including full bootstrap, or during the page rendering process" means. Can you cite some examples? Would "touches includes/bootstrap.inc" suffice?

Could possibly say "any code in /includes, hook_boot() or hook_init()" except there is lots of rarely called code in /includes where we probably don't care much. I would say if no-one in an issue knows where the code they are modifying actually gets called or how frequently, that is a serious problem (both for pointless micro-optimizations where they make no difference, and regressions in the critical path). If this gates thing works, the only way is if people are actually considering questions like that - the examples are broad because Drupal code paths are not predictable in most cases.

"System calls" -- centralized loading or scanning functions: could you cite some examples here? (I assume you mean things like module_invoke_all() or file_scan_directory(), but not sure.)

Yeah that's right, examples would be good to add.

Front end is vague because I have least experience there, hoping someone who does can flesh that out.

I'm concerned about the documentation burden here, at the rate this is going the gate is going to require 30-odd pages of in-depth documentation. What tools get used for performance testing varies over time - xhprof didn't exist in early 2008, services like Browsermob are even younger, many tools are only available on certain OS etc.

So any documentation that is too much tied to how to use specific tools risks getting dated quite fast, or being counter productive. There's also plenty of docs around on the internet on how to use most of this stuff, we don't necessarily need to duplicate all of this on Drupal.org.

On the other side there is 'how to write performant code' - but most of this stuff makes no sense in isolation, and most pages I see about it are stupid voodoo micro-optimization that doesn't take context into account. We can get around that issue with a "what not to do" and "Drupal performance regressions we love to hate" kind of stuff but anything that says "do this, don't do this" is unlikely to be useful - you can still mess things up, or be fine, whether you follow particular guidelines or not.

If we're going to get anything from this gate it should be this:

• when people are working on core patches, they bear performance and scalability in mind, at all.

• when changing code, the frequency of evaluation of that code should be part of the criteria for assessing performance impact.

• the performance impact of changes is measured using appropriate tools (not guessing, not ab -c1 -n500 for every single change regardless of whether it gives meaningful results or not)

• the performance impact of changes is understood in terms of how it relates to resource usage (rather than raw requests per second in a limited benchmark) - this means memory, cpu, file system i/o, database, network calls from within the application, cache invalidation, stampedes, network use between the user and the browser, browser performance itself.

• the performance impact of changes is understood in terms of how it relates to how Drupal sites may run into issues (large data sets, lots of pages, lots of modules, cold starts etc.)

If those start to come into place, then we should see less regressions, but I don't think it possible to come up with a set of rules that will prevent regressions going in - if we tried to do it like code style, core would massively fail already.