CRM-20455 - DB_common::modifyQuery() - Optimize

[totten]

Overview

The recently merged PR #180 added modifyQuery() to allow dynamic inspection/modification of most SQL queries (post-boot).

This patch modifies and micro-optimizes the new (unreleased) interface.

Ordinarily, one wouldn't care about this level of micro-optimization; however, it's entirely foreseeable that this code will run hundreds or even thousands of times while a user works with a single page.

Depends: civicrm/civicrm-core#14882, civicrm/civicrm-core#14883

Before

For each typical SQL query, the call to modifyQuery() requires:

• Constructing an instance of GenericHookEvent, which does some array shuffling and encourages reflective code.
• Performing multiple lookups of the container/dispatcher system

After

For each typical SQL query, the call to modifyQuery() requires:

• Constructing an instance of QueryEvent, which is a lighter object
• Fewer lookups (isset(Civi::$static[...]))

Comments

I did some simple benchmarks -- running 10,000 simple SQL queries with various forms of modifyQuery(). The baseline (no events) had the fastest average (9.444s). Adding GHE bumped runtime up +9% (10.298s). With this patch, it only adds +3.5% (9.787s).

The averages are based on 5 samples each. For full details, see https://gist.github.com/totten/da21a8386a7ddcc62def5d78e912bf8c

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

thanks @totten 300ms over 10k queries seems OK to me & def better!

[seamuslee001]

Error was

/home/jenkins/bknix-dfl/build/pkg-259-592jo/web/sites/all/modules/civicrm/packages/DB/common.php).
Error: Class 'Civi\Core\Event\QueryEvent' not found in DB_common->modifyQuery() (line 1162 of /home/jenkins/bknix-dfl/build/pkg-259-592jo/web/sites/all/modules/civicrm/packages/DB/common.php).
Drush command terminated abnormally due to an unrecoverable error.       [error]

which makes sense as the core PR hadn't been merged

[seamuslee001]

Jenkins re test this please

[totten]

Cool, the second test-run showed that 14882 fixed the QueryEvent error. It also revealed some test-harness issues, which are addressed in another PR: civicrm/civicrm-core#14883 . We'll need another test-run after that's merged.

[eileenmcnaughton]

test this please

[eileenmcnaughton]

Just a note that I'm doing some performance tests before deploying some updates.

My first tests on 5.13 were importing (via script) around 462 contacts with contributions per minute. On 5.17 both with and without using this query modifier I was getting around 484 contributions per minute.

I don't consider the above to be reliable enough to say 5.17 is definitely faster than 5.13 but I think it is reliable enough to say the query modifier is not dragging us down