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.
Currently module_implements relies on module_list (which in itself needs love), this patch makes it rely on the registry instead. This should make db_merge possible during bootstrap on postgresql which currently wont work.
Comment | File | Size | Author |
---|---|---|---|
#88 | module_implements.patch | 17.82 KB | Anonymous (not verified) |
#86 | module_implements.patch | 15.86 KB | chx |
#83 | module_implements.patch | 17.07 KB | Anonymous (not verified) |
#81 | module_implements.patch | 15.41 KB | chx |
#72 | module_implements.patch | 17.6 KB | Anonymous (not verified) |
Comments
Comment #1
Dave Reidpatch -p0 < module_implements.patch
patching file includes/bootstrap.inc
Hunk #1 succeeded at 820 (offset -11 lines).
Hunk #2 succeeded at 1430 (offset -11 lines).
patching file includes/common.inc
Hunk #1 succeeded at 1526 (offset 29 lines).
patching file includes/menu.inc
Hunk #1 FAILED at 1719.
1 out of 1 hunk FAILED -- saving rejects to file includes/menu.inc.rej
patching file includes/module.inc
patching file includes/registry.inc
patching file index.php
patching file modules/system/system.install
Hunk #1 succeeded at 1091 (offset -4 lines).
Comment #2
chx CreditAttribution: chx commentedReroll.
Comment #3
chx CreditAttribution: chx commentedFor real :)
Comment #4
Dave ReidDB errors abound on my current test install (even after update.php), so I tried on a fresh install. Installed correctly and everything looks good except an error on the bottom of every page: Fatal error: Call to undefined function registry_cache_hook_implementations() in /home/davereid/Projects/drupal-head/includes/common.inc on line 1529. Tried running the system test. Registry parse file test fails 0/3 and stops:
Comment #5
chx CreditAttribution: chx commentedThat's an easy fix for common.inc, no idea at all how that call remained in there. I think I fixed the test as well -- it's also clear that we will need a new test. Meanwhile, reviews are still welcome.
Comment #6
Dave ReidNow getting following error on bottom of each page: Fatal error: Call to undefined function registry_cache_path_files() in /home/davereid/Projects/drupal-head/includes/common.inc on line 1530.
I've attached a revised patch. I think someone that has a little more understanding of the registry code to comment on the actual patch. I'll work on learning about it...
Comment #7
Dave ReidErr...didn't mean to change status.
Comment #8
chx CreditAttribution: chx commentedWell, #6 is wrong, that function should not be removed. The patch comes from total module revamp and this was an accident.
Comment #9
chx CreditAttribution: chx commentedSilly me, forgot to add the install files to the registry. Works pretty well!
Comment #10
Crell CreditAttribution: Crell commentedSubscribe. Will try to review at some point.
Comment #11
chx CreditAttribution: chx commentedAll tests now pass. And do you need any proof that patch my speeds up things? Check tracker.test. That was some test bug to hunt down. Little drop became a bit too fast :D
Comment #12
Crell CreditAttribution: Crell commentedI am either totally confused as to what you're trying to do or there are way too many patches rolled into one here. There's whitespace cleanup, adding .install files to the .info files, what looks like registry changes, and what looks like adding Yet Another Boolean Parameter (YABP). Only the last seems to have any relation to this issue, and I am actually against that approach in most cases for DX reasons.
IIRC from DBTNG, all that needed to be done for this issue was making module_list() itself not suck. Fix that to not be moronic and the rest fixes itself. Let's just do that and do the registry cleanup in another patch.
Comment #13
chx CreditAttribution: chx commentedMore kittens are saved by moving the tracker test patch to http://drupal.org/node/309951 . For dropping sort, there was no point in calling watchdog or menu in an alphabetical order so dropping the sort argument is just cleanup. Yes that's makes the patch non-kitten free, but it'd be rather daunting to submit a patch which patches out $sort from the current implementation of module_implements just to be replaced by this version the next day. If we are bent on keeping $sort I can put it back.
Comment #14
Crell CreditAttribution: Crell commentedI think we crossposted.
Comment #15
chx CreditAttribution: chx commentedSorry, missed Crell's reply. So this patch makes module_implements rely on the registry instead of module_list. Or you can say that we save some queries to the registry table when the hook registry for this router path is not yet cached. The patch needs to add the install files to the info otherwise hook_schema would fail as the registry wont contain them. To do this we needed a few registry.inc changes where the module name is passed all along. This way when we see a function name which starts with the name of the module, the rest can be a hook, so we store it as such. The rest of the registry changes is just removing what seemed to be cruft at the end of the day. Dunno what YABP are you talking of, really.
Comment #16
chx CreditAttribution: chx commentedOh and by the way -- this patch paves the way for per hook weights.
Comment #17
webchickThis issue is a pretty good example of http://webchick.net/patch-reviewers-are-not-clairvoyant. chx spent about an hour with me in #drupal walking me through the patch. Here's a summary:
Comment #18
webchickNow, for an initial pass.
At the very least they need comments, but it really is starting to feel like module_implements() is becoming this Frankenstein monster of a function -- it's module_build_hook_cache(), module_clear_hook_cache(), module_get_implementation_of_hook(), at least -- and should likely be split up.
That parameter either needs WAY more comments, or it needs to just be $module.
Comment #19
chx CreditAttribution: chx commentedmodule_implements split up, utility functions added, doxygen'd commented. That parameter is fixed.
Comment #20
RobLoachWas hitting:
Fixed that with:
Now I'm hitting:
Comment #21
chx CreditAttribution: chx commentedFixed.
Comment #22
RobLoachHmmm,
.....
Comment #23
chx CreditAttribution: chx commentedReadded $sort via another utility function after realizing help needs it.
Comment #24
sunRe-rolled without unnecessary cruft (coding-style changes).
Comment #25
sunFixed some PHPdocs, and renamed _module_implements_sorted() to _module_implements_sort(), and changed $stored into $cache in _module_implements_sort() for better visual separation from $sorted.
Comment #26
sunTo test this patch on a existing site running HEAD, execute the following queries:
and visit devel/cache/clear afterwards.
Testing code in custom.module:
Comment #27
sunRemoved some obsolete cruft from _registry_parse_files().
Fixed _element_info() returning NULL instead of an (expected) array if no elements are returned. (leading to a fatal error otherwise)
Please note that you still need to perform a complete registry rebuild after applying this patch.
Testing results using the example code in #26 (which should simulate the situation we have DIRECTLY after installation):
For D6:
For D7 unpatched:
For D7 patched:
Simpletest results:
5799 passes, 5 fails, 0 exceptions
Like always, failed tests are only file related; see #315520: assertFilePermissions() not working on Windows
Aside from that, the implementation looks good, clever, and clean. Some parts might not be trivial, but make perfectly sense in the end.
So this patch should be RTBC.
The only remaining issue - NOT relevant/tied to this issue - is whether we can implement some failsafe algorithm, which automatically performs a complete registry rebuild if the stored data is empty, unavailable, or incomplete due to any reason, since users need to invoke drupal_flush_all_caches() somehow in such situations.
Comment #28
chx CreditAttribution: chx commentedNote that if registry is empty it rebuilds itself. You only had a problem because you ran an unpatched Drupal and then a patched one and the cached contents clashed. This can not happen normally.
Comment #29
drewish CreditAttribution: drewish commentedsubscribing
Comment #30
webchickI've reviewed this patch 2-3 times now, and all the nit-picky things I can find to say about it have been fixed.
At an architectural level, I do have some concerns about the 'best guess' approach it takes to identifying hooks. But I understand that this is a trade-off in agility vs. accuracy... there are concerns about requiring yet another "registry" function with the theme registry, and possibly variable registry, etc.
However, I'd like to get Dries's two cents before committing this, since it seems like it crosses that treshold of "big" change.
Comment #31
Anonymous (not verified) CreditAttribution: Anonymous commentedi don't like the extra 'guess' hook data in the registry schema either.
here's an alternative for
_module_implements_build
that doesn't require any changes to the registry schema.its a bit slower than chx's patch, but its simpler, and it is quicker than the current implementation.
Comment #32
webchickSounds like this still needs discussion.
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedto make this easier, i've created add functions in .info files to the registry.
Comment #34
chx CreditAttribution: chx commentedNo this does not, the
IN (" . implode(',', $candidate_functions) . ")");
is slow and as you have more modules it gets slower. A simple query against an indexed hook column is vastly faster.Comment #35
chx CreditAttribution: chx commentedRerolled after the patch mentioned in #33 is in.
Comment #36
Anonymous (not verified) CreditAttribution: Anonymous commentedupdated patch always writes the module implementations to cache if we don't get any cache data for a request.
as it was, if we didn't get anything from the cache, then this snippet from
module_implements
would always be false, and we wouldn't cache when we should.
all tests pass, and db queries for a clean install, all modules enabled hitting node/1 with 1 comment are cut by 80%.
still don't like the hook guess approach, and i'll likely post an alternative soon, but this is a big win for cutting back queries, so even if i don't convince people that we don't need the hook guess stuff, i'd still like to see this go in.
setting to code needs review - someone else should validate the caching change.
Comment #37
chx CreditAttribution: chx commentedThe change is good, yes but you can express it with a bit less code. Thanks. That drop in 80% of queries is just before the hook cache hits in but it's still great.
Comment #38
keith.smith CreditAttribution: keith.smith commentedThis is minor enough that someone could do it before a commit so I'll leave this RTBC; I think "settings" in the comment above should perhaps be "setting".
Or something like "By default, modules are ordered by weight and filename. If this is option is TRUE, modules are ordered alphabetically by module name." might be just as clear, or more so.
Comment #39
webchickUndefined index: lg_debug Notice database_test.test 1689 DatabaseLoggingTestCase->testEnableLogging()
Comment #40
webchickWrrrooonnnng issue. :) Sorry.
Comment #41
webchickAs I said to chx on IRC, this qualifies as a patch that I would like to get Dries's OK on before I commit it. It's kind of dirty, and he might have a better solution.
Comment #42
John Morahan CreditAttribution: John Morahan commentedI tried to make use of this by turning custom_url_rewrite* into hooks, and ran into bizarre problems with simpletest while doing so. This line in tearDown() in drupal_web_test_case.php seemed to be the culprit:
Should that be using the new module_implements(MODULE_IMPLEMENTS_CLEAR_CACHE) instead, or as well?
Comment #43
Anonymous (not verified) CreditAttribution: Anonymous commentedupdated patch to keep up with HEAD. all tests pass.
@John Morahan - not sure about your question.
Comment #44
Anonymous (not verified) CreditAttribution: Anonymous commentedsimpler version, merged
_module_implements_build
and_module_implements_check
and simplifiedmodule_implements
.all tests pass. setting back to code needs review to get some more eyes on the changes.
Comment #45
Anonymous (not verified) CreditAttribution: Anonymous commentedfollow up comment following a request from chx to clarify what i changed.
i've modified the code so that for a cache miss for a given hook, we just call one function,
_module_implements_build
._module_implements_build
pulls the hooks from the db, and loads the file for the hook into memory if its not already loaded.this means we don't need the
$loaded
static or_module_implements_check
helper.so, just a simplification really. returning the favour for once, as chx usually simplifies my code ;-).
Comment #46
chx CreditAttribution: chx commentedJustin, that won't fly, the reason I had the loaded thing is that it's possible that the implementations are cached but the include file containing the hook is not loaded.
Comment #47
Damien Tournoud CreditAttribution: Damien Tournoud commentedReview of chx patch from #37:
What is the point of this (is that scope-creep)?
The calling pattern of this function looks unecessary complicated. Why not replacing $sort with a bit mask $mode with MODULE_IMPLEMENTS_CLEAR_CACHE, MODULE_IMPLEMENTS_WRITE_CACHE and MODULE_IMPLEMENTS_SORT?
What is the use of forcing the load of the function here?
I don't see the point of moving the whole _module_implements_check() in a separate function (it is called only once), it could be moved back to module_implements().
In that case, this code should be removed.
Here we are storing cached implementation in $cache just for the purpose of clearing those statics. I suggest moving those statics to module_implements().
Comment #54
chx CreditAttribution: chx commentedMoving to separate functions IMO increased the readibility.
Comment #55
chx CreditAttribution: chx commentedRe. form.inc , it can be omitted if you want, it only caused problems for sun because he applied the patch into an already installed Drupal. I removed six comments attempting to derail the issue over such a minute detail.
Both to Damien and Justin,
module_implements
must ensure that the function you are likely to be calling in the near future is actually loaded.About calling patterns, as you wish, really. I hope I can get back Justin in the issue and agree whether #43 or #44 is to be continued.
Comment #56
Anonymous (not verified) CreditAttribution: Anonymous commentedchx: #43. i missed the 'if its loaded from cache the implementations might not be in memory' part, so #44 is flawed.
Comment #57
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe agreed with chx on the IRC that we could remove both _module_implements_check and _module_implements_build and factor them inside module_implements while keeping high readability.
This patch still misses two key parts:
- (1) the default ordering of hooks by weight and module name
- (2) a $sort parameter for _module_implements_maintenance
Comment #58
chx CreditAttribution: chx commentedHere we come. I removed the form.inc patch so kindly please apply this and install after.
Comment #59
chx CreditAttribution: chx commentedNow, registry.test needed an update. Also, if you notice the 68 for the suffix length, well MySQL did not allow longer otherwise the index creation would fail. We can make the weight a smallint to win a few bytes and juggle with the module and the suffix but meh -- when did you use a hook longer than 68 characters?
Comment #60
keith.smith CreditAttribution: keith.smith commentedAs in #38, I think "settings" should be "setting".
Comment #61
Damien Tournoud CreditAttribution: Damien Tournoud commented- acked #60
- replaced $cache (the whole previous cache) to $cached_hooks (the count of previously cached implementations), to save memory
I think it would be better if we limit module names to 128 chars. That way, hooks could be 126 characters (because module + '_' + hook = 255, because function names are stored in VARCHAR(255)), and we would be better off here.
Comment #62
chx CreditAttribution: chx commentedI agree with Damien though PHP reference counting makes his memory argument moot (unless there is a new hook called) -- instead it's a performance improvement, and as Zend hashtables store their count storing the count in a variable is a rather speedy operation.
Comment #63
sunRe-rolled,
- minor PHPdoc fixes for module_implements and _registry_parse_file.
- changed $hook === MODULE_IMPLEMENTS_CLEAR_CACHE tests to be type-agnostic, so invoking module_implements('', ...) does not result in a cleared cache.
Comment #64
Anonymous (not verified) CreditAttribution: Anonymous commentedis this safe?
what if we have a one module enabled and an another module disabled, and the count of hooks stays the same, but the list if different?
Comment #65
Damien Tournoud CreditAttribution: Damien Tournoud commented@justinrandell: You have to understand that this function does a progressive caching of the hook. Implementations for a given hook are cached the first time the hook is called. That's the reason why we check for the number of implementations here. Module enabling/disabling is a completely different story. In that case, we simply empty the full cache of hook implementations.
Comment #66
Anonymous (not verified) CreditAttribution: Anonymous commented@Damien: yep, my mistake.
Comment #67
Anonymous (not verified) CreditAttribution: Anonymous commentedwatching the registry queries is much easier with the new db logging code :-)
with the patch at #63 we cut out a lot, but for a logged in user, all core modules enabled, loading node/1 with one comment, we're still at 12 registry queries with a fully loaded cache.
if we cache registry misses, this drops down to 5.
if people aren't to allergic to the idea, i'll roll a patch, else i can wait for a follow up issue.
Comment #68
chx CreditAttribution: chx commented@justinrandell thanks for filing a separate issue at http://drupal.org/node/325665 ;)
Comment #69
Anonymous (not verified) CreditAttribution: Anonymous commentedok, lets do the registry misses caching in a follow up.
here's an updated patch, with
drupal_function_exists
calling_registry_check_code('function', $function);
. otherwise, its the same as at #63.all tests pass.
now, how do we get Dries to weigh in on this issue?
Comment #70
Anonymous (not verified) CreditAttribution: Anonymous commentedattached patch this time.
Comment #71
Damien Tournoud CreditAttribution: Damien Tournoud commented@justin: I really don't like those changes added a the last minute.
Your change broke the negative caching in drupal_function_exists(), because _registry_check_code() returns NULL on miss, not FALSE.
Comment #72
Anonymous (not verified) CreditAttribution: Anonymous commented@Damien: good catch on NULL vs FALSE, attached patch fixes that,
_registry_check_code
return FALSE on a miss.all tests pass.
Comment #73
Damien Tournoud CreditAttribution: Damien Tournoud commentedOpened #325994: Reduce system.name to 128 chars for the suggested change to the module name length.
Comment #74
kbahey CreditAttribution: kbahey commentedI tried to benchmark using this patch, to see if it speeds up things, using an existing Drupal 7 site (from last Friday or so), but I get the following error when trying to access any page:
And there are no pending updates in update.php.
So, I emptied the database, loaded the Drupal 6 database, and tried to run update.php, but I get another error and can't proceed:
The error page linked is not much better, very vague:
So, this broke update.php.
Backing out the patch makes update.php works again.
I tried creating an system_update_7012() for the new fields and indexes for the registry table, but that caused update.php to white screen (probably a PDO error ...)
Comment #75
Anonymous (not verified) CreditAttribution: Anonymous commentedjust some simple data points of HEAD vs HEAD + this patch.
using ab -n 1000 -c 10 http:///node/1
APC on, page and block caching off, anonymous user, all core modules enabled, one comment on node/1:
HEAD
Requests per second: 27.88 [#/sec] (mean)
Requests per second: 27.74 [#/sec] (mean)
Requests per second: 27.59 [#/sec] (mean)
patched
Requests per second: 30.62 [#/sec] (mean)
Requests per second: 30.50 [#/sec] (mean)
Requests per second: 30.69 [#/sec] (mean)
Comment #76
Dries CreditAttribution: Dries commentedjustin: how does that compare to a Drupal 6 site? Would be useful, but not critical, to know.
Comment #77
Dries CreditAttribution: Dries commented- Overall this looks like a good API clean-up. The only part that smells a bit is the maintenance version of module_implements. The documentation of that falls short: "This is the maintenance version of module_implements." Please add some more documentation description why this function is important/needed, and when it should (or should not) be called. By adding some more comments, we might be able to increase the comfort level of having such a function.
-
Maybe add a code comment explaining what we do? I understand what we do, but it would saved me 15 seconds if there was a code comment. ;-)
- Typo:
It'the name
.Looks good -- it is a matter of adding a bit more documentation so we can make sense of the maintenance version.
Comment #78
kbahey CreditAttribution: kbahey commented@Dries & Justin
I was trying to exactly do that: compare performance of before/after the patch and to Drupal 6
I wanted to keep all factors the same by upgrading the database from D6 to D7.
See my comment in 74.
If we can get over this, I can do more thorough performance analysis.
Comment #79
chx CreditAttribution: chx commented@kbahey, there are issues open for update.php please do not derail this one, i will add more docs and add an underscore to the maintenance version as you should never call it.
Comment #80
kbahey CreditAttribution: kbahey commented@chx
Maybe I was not clear.
Without the patch, I can update a site fine.
With the patch, I cannot update a site at all.
So the patch introduced something that causes failure. So was pointing this out.
I am fine that the update be fixed in a separate issue, just wanted to point this out in case it can be easily fixed.
Comment #81
chx CreditAttribution: chx commentedWait a second, I already had an underscore there. Ah well. It needed a reroll anyways and added "for internal use only".
Comment #82
Anonymous (not verified) CreditAttribution: Anonymous commented@Dries, repeated with DRUPAL_6 vs HEAD vs HEAD + this patch
using ab -n 1000 -c 10 http://site/node/1
APC on, page and block caching off, anonymous user, all core modules enabled, one comment on node/1:
DRUPAL_6
Requests per second: 37.50 [#/sec] (mean)
Requests per second: 37.60 [#/sec] (mean)
Requests per second: 37.56 [#/sec] (mean)
HEAD
Requests per second: 27.58 [#/sec] (mean)
Requests per second: 27.63 [#/sec] (mean)
Requests per second: 27.60 [#/sec] (mean)
HEAD + patch
Requests per second: 30.64 [#/sec] (mean)
Requests per second: 30.58 [#/sec] (mean)
Requests per second: 30.66 [#/sec] (mean)
APC off, page and block caching off, anonymous user, all core modules enabled, one comment on node/1:
DRUPAL_6
Requests per second: 9.42 [#/sec] (mean)
Requests per second: 9.44 [#/sec] (mean)
Requests per second: 9.45 [#/sec] (mean)
HEAD
Requests per second: 7.96 [#/sec] (mean)
Requests per second: 7.91 [#/sec] (mean)
Requests per second: 7.93 [#/sec] (mean)
HEAD + patch
Requests per second: 8.34 [#/sec] (mean)
Requests per second: 8.36 [#/sec] (mean)
Requests per second: 8.34 [#/sec] (mean)
note, this is not an upgrade of the 6 install to 7, its two clean installs, enable all modules, create one node and add a comment to the node. clearly HEAD with or without the patch is slower, inline with the results from khalid.
Comment #83
Anonymous (not verified) CreditAttribution: Anonymous commentedrerolled to keep up with HEAD, and added a couple of lines of comments to the hook gathering code in the registry rebuild cycle as per Dries comments in #77.
so, no code changes, and all tests still pass, so RTBC.
Comment #84
Anonymous (not verified) CreditAttribution: Anonymous commentedahem, RTBC.
Comment #85
Dries CreditAttribution: Dries commented_module_implements_maintenance should explain how it is different from the regular _module_implements and why it is a safe path. Almost there ...
Comment #86
chx CreditAttribution: chx commentedIt's safe because it does not use the DB. Explained. In detail.
Comment #87
keith.smith CreditAttribution: keith.smith commentedAs Dries pointed out in #77:
And, as in #38 and #60,
there's something odd with "settings" here.
But, these are both minor issues; I'll roll a patch if someone doesn't beat me to it.
Comment #88
Anonymous (not verified) CreditAttribution: Anonymous commentedgeez, you'd think people around here actually cared about code quality or something :-)
updated patch attached to address typo's in #87.
Comment #89
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks guys.
Comment #90
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.