Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
When listing formats - i.e. a comment form on node view, we do an individual query for each format. Attached patch stops that by building the array of filters the first time we call the function, so that subsequent calls only fetch from static instead. Needs some cleanup since variable naming gets a bit tangly in there, but removes two queries on a default 'article' view in HEAD.
Comment | File | Size | Author |
---|---|---|---|
#38 | drupal.filter-list-format.37.patch | 25.68 KB | sun |
#36 | drupal.filter-list-format.36.patch | 25.69 KB | sun |
#33 | drupal.filter-list-format.33.patch | 23.24 KB | sun |
#31 | drupal.filter-list-format.31.patch | 19.26 KB | sun |
#29 | drupal.filter-list-format.29.patch | 18.98 KB | sun |
Comments
Comment #2
catchNew patch, fixed tests, added caching for the query, avoids doing the extra work on the filters up front in case they don't actually get used.
HEAD:
Executed 72 queries in 45.77 milliseconds.
Patch:
Executed 70 queries in 46.68 milliseconds
Comment #3
catchAnd the patch. I'm not sure why we call filter_list_formats() with an id of 0 on admin/config/content/filter/add, for now added a line to protect against this, not ideal but maybe sun can take a look?
Comment #4
Dries CreditAttribution: Dries commentedSo, we reduced the number of queries, but we actually got slower?
Comment #5
sunsubscribing
I need to think about this edge-case.
Comment #6
catch@Dries, devel query timings has something like a 50-100% standard deviation and isn't to be trusted unless it's dozens of queries or a huge filesort or something - page execution time is similarly unreliable, so those pastes are only for the query count (which is reliable as long as you have a warm cache).
It's also likely that benchmarks won't be conclusive with just these couple of queries, although I'm happy to run them.
The reason I post these patches is because:
* Less round trips to the database and less PHP overhead executing queries is worth having even when the queries themselves are small and indexed.
* 2-3 queries quickly adds up when it's multiplied across multiple issues and subsystems.
Comment #7
sunWhatever we do, we want to remove this condition.
Actually, I think that invoking filter_list_format() for new formats is totally wrong, so this bears a fix elsewhere, which we may want to include here, not sure.
Comment #8
catchIn this case, devel was accurate, because I was missing an index change on the {filter} table, leading to a 10ms query, whoops!
Thought of a benchmarking scenario, not the most realistic in the world but not impossible either.
I added a bunch of input formats so that were 15 enabled on the site, gave anonymous user access to 14, access to and view comments, then benchmarked node/1 ab -c1 -n1000
HEAD:
Requests per second: 9.83 [#/sec]
Executed 65 queries in 36.52 milliseconds.
With patch:
Requests per second: 10.51 [#/sec]
Executed 52 queries in 23.04 milliseconds.
I don't know any sites with 14 input formats, but I do know some with 6 or more. So that 7% change probably equates to 2-3% on a more realistic site.
We still need to resolve the admin/config/content/filter issue though per #7.
Comment #9
sunI need to see what breaks.
Comment #11
sunHere we go.
Comment #13
catchLooks fine.
Index didn't make it into any of the patches, properly added this time.
Same benchmark as above, this time with 5 formats.
HEAD: 8.80 [#/sec]
Patch: 9.10 [#/sec]
Comment #14
sunThis patch nicely exposes a bug I already encountered in #394466-28: Text format tips do not explain that HTML is allowed when HTML filter is disabled, but didn't create an issue for yet.
filter_format_save() needs to save all available filters for a text format, regardless of their status.
This patch should hopefully fix it.
Comment #15
catchCross-posted with the bot.
Comment #16
suncross-posted. Merged in that index change.
Comment #17
catchAnd with sun. This patch merges #13 and #14.
Comment #18
catchomg. Either of those two should be fine :p
Comment #19
sunLOL :)
Nope, yours uses a strange update function number :P
Let's hope that tests pass on #16 and be done with it :)
Comment #21
catchJust ran #16 locally and it has the same failures.
Comment #22
sunok, that was quite a challenge. I'm totally happy that we have *so extensive tests* for filter.module. Changing this code lead to subsequent different fails in filter tests, which is awesome.
Comment #24
sunoh nice, this also requires us to finally fix the installer. :)
Comment #26
sunAlso fixing PHP module's raw data query insertions...
Comment #27
sunComment #29
sunComment #31
sunComment #33
sunHoly cow!
This turned into a true bugfest.
Comment #34
sunerm, considering all the filter system and installation profile bugs that are fixed in this patch, bumping to critical.
Comment #36
sunThanks, catch! You saved us from monster post-release headaches!
Comment #37
Dries CreditAttribution: Dries commentedI spent some time looking at this and it looks good. The latest patch doesn't seem to apply though, and might need a quick reroll. Otherwise RTBC, IMO.
Comment #38
sunRe-rolled against HEAD.
Also, to summarize these changes:
Comment #39
sunNote that some of the critical issues I mentioned in the summary would not exist with #573852: Rename {filter_format}.name to .title and replace .format with machine-readable .name - i.e. if modules and tests were able to refer to text format "php_code" or "filtered_html" instead of MAX(format) or 1. But unfortunately, that's way too much for D7, because it would require us to change all 'format' columns throughout core, and on top of that, it would conflict with my plans for D8. :-(
Erm, so this was just FYI. ;)
Comment #40
catchLooks fine here, Dries already reviewed, let's get this in before it reaches 40k ;)
Comment #41
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Great job.