The NodeGrantDatabaseStorage::alterQuery() method appears to be doing extra work which could be avoided: it builds a constant array of grants in a loop on all tables in a query instead of building it just once:
$grants = node_access_grants($op, $account);
foreach ($tables as $nalias => $tableinfo) {
$table = $tableinfo['table'];
if (!($table instanceof SelectInterface) && $table == $base_table) {
// Set the subquery.
$subquery = $this->database->select('node_access', 'na')
->fields('na', ['nid']);
// If any grant exists for the specified user, then user has access to the
// node for the specified operation.
$grant_conditions = static::buildGrantsQueryCondition($grants);
Some CPU use could be avoided by factoring the buildGrandsQueryCondition out of the loop, as that is a pure function which takes invariant arguments.
Similarly references and calls to \Drupal::languageManager()->isMultilingual()) are invariant and could be taken out of the loop.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | interdiff_3-7.txt | 2.1 KB | init90 |
| #7 | optimize_AlterQuery_in_NodeGrantDatabaseStorag_3066637_7.patch | 2.06 KB | init90 |
| #3 | 0001-Issue-3066637-optimize-NodeGrantDatabaseStorage-alte.patch | 3.13 KB | fgm |
Comments
Comment #2
fgmComment #3
fgmSuggested patch. Let's see if this causes any unexpected fail.
Comment #4
ndobromirov commentedWhy the rename of
subquerytosub_querythis makes a lot of noise on the change that is done here.What was the potential win here if you can share some benchmarks?
Other than than I would RTBC it, as the rename can be fixed on commit.
Comment #5
fgmThe rename was to apply current coding standards, since the patch was already touching the same loop, but is indeed otherwise unrelated.
The win is likely to be very small, as explained in the issue, sub-millisecond on each query, only meaningful on non-core-default sites with at least one access control implemented (ideally several e.g. domain + content_access) and pages with many node list requests, because this is where this will actually run many times.
Regarding the fails, the MySQL 8 one appears to be a bot fluke, and the PG one seems to be an error in core in the array comparison from an unordered map.
Comment #6
catchLet's drop the rename here before commit and just do the main change, it's touching lines that otherwise wouldn't be affected by the patch.
Comment #7
init90I've done changes according to last comment.
Replaced '$sub_query' to '$subquery'.
Also, I've added small additional change - renamed $multilingual variable to $is_multilingual for consistency with method name.
Since my changes were trivial, I'm returning status to RTBC.
Comment #8
tstoecklerThanks, looks great. RTBC++
Comment #9
catchCommitted 0d11203 and pushed to 8.8.x. Thanks!
Comment #11
catch