| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | node.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (duplicate) |
Issue Summary
Every page load the node types are build 'invoke_all('node_info')'. Therefor every page load the descriptions of the node types are prepared with t(''). The locale module only caches strings shorter then 75 characters.
The following node types have long descriptions:
Blog - 78 - A blog entry is a single post to an online journal, or blog.
Poll - 191 - A poll is a question with a set of possible responses. A poll, once created, automatically provides a simple running count of the number of votes received for each response.
Forum - 85 - A forum topic is the initial post to a new discussion thread within a forum.
Trigger - 367 - Triggers are system events, such as when new content is added or when a user logs in. Trigger module combines these triggers with actions (functional tasks), such as unpublishing content or e-mailing an administrator. The Actions settings page contains a list of existing actions and provides the ability to create and configure additional actions.
So every page load locale does this 4 queries though they ain't displayed.
Can we fix this by:
- Chunk the strings in parts of 75 chars?
- Get the descriptions from node_info only when they are used?
- Make the descriptions not langer then 75 chars?
I didn't know what component of Drupal affects this because it is a general rule for all node types, therefor I set it on 'other'.
Comments
#1
#2
I though those were in the database, and thus not translatable?
#3
Could be, but because every page load the hook_node_info is called it also tries to translate the description with t('').
And because the ones of that node are bigger then 75 chars it ain't cached so causes 4 query's every page load.
#4
Looked into it a little bit more. In node_invoke, node_menu, node_access and more the function 'node_get_types()' is used. This builds the node type list when it has not been made yet. It calls the function '_node_types_build()'. So almost every page loads the node types (you only need to do a node_load or a new menu rebuild or whatever).
/**
* Builds and returns the list of available node types.
*
* The list of types is built by querying hook_node_info() in all modules, and
* by comparing this information with the node types in the {node_type} table.
*
*/
function _node_types_build() {
$_node_types = array();
$_node_names = array();
$info_array = module_invoke_all('node_info');
foreach ($info_array as $type => $info) {
$info['type'] = $type;
$_node_types[$type] = (object) _node_type_set_defaults($info);
$_node_names[$type] = $info['name'];
}
$type_result = db_query(db_rewrite_sql('SELECT nt.type, nt.* FROM {node_type} nt ORDER BY nt.type ASC', 'nt', 'type'));
while ($type_object = db_fetch_object($type_result)) {
// Check for node types from disabled modules and mark their types for removal.
// Types defined by the node module in the database (rather than by a separate
// module using hook_node_info) have a module value of 'node'.
if ($type_object->module != 'node' && empty($info_array[$type_object->type])) {
$type_object->disabled = TRUE;
}
if (!isset($_node_types[$type_object->type]) || $type_object->modified) {
$_node_types[$type_object->type] = $type_object;
$_node_names[$type_object->type] = $type_object->name;
if ($type_object->type != $type_object->orig_type) {
unset($_node_types[$type_object->orig_type]);
unset($_node_names[$type_object->orig_type]);
}
}
}
asort($_node_names);
return array($_node_types, $_node_names);
}
As you can see it invokes a hook_node_info there for a lot of modules which specify node_info are loaded:
function forum_node_info() {return array(
'forum' => array(
'name' => t('Forum topic'),
'module' => 'forum',
'description' => t('A <em>forum topic</em> is the initial post to a new discussion thread within a forum.'),
'title_label' => t('Subject'),
)
);
}
The description is too long (75 chars) and there for is not cached in the cache 'locale:LANGCODE' and queried almost each page load though it ain't displayed.
#5
This really looks overkill. We should cache the result of
_node_types_build(), and invalidate it when $reset is TRUE innode_get_types(). Does anyone see a reason why we shouldn't?#6
You mean as a seperate cache entry, or in variables? If you want I can do some tests tonight
#7
This looks like a good find. Damien - do you mean a static?
#8
The node types are already static in node_get_types. This patch makes node_get_types uses cache when available. Since Drupal has alot of rebuild node types functions I used that ones to reset it. I runned the node module tests and they still worked fine.
#9
This needs more work.. currently just one cache is saved though it should be saved using the languagecode.
#10
New patch with language code.
#11
We are on the good way, here.
On code review, I don't think we really need
node_types_get_cache_cid(): $language is set whether or not the locale module is enabled. So you could easily use'node_types_' . $language->->languageunconditionally as the cid.The principle of clearing the cache *only* on $reset (and not on every page load) needs more review. I'm unsure of the impact of that change.
#12
New patch is up which just uses $language->language;
Clearing this each page load would make this patch useless. The idea is that we don't just save the node types in a static like node_get_types() does but save it in cache. How much times are your node types changed? And if so build is called with the $reset parameter.
The reason I want to cache this is because it is doing:
1. module_invoke_all
2. A slow query
3. For each node type with a description longer then 75 char a query to get the translation
So that should be slower then just a unserialize from cache!
#13
@R.Muilwijk: I want to be sure that $reset is called wherever it is appropriate. In the current situation we have a failover: even if $reset is not called, the static cache will be lost at the next page load... Are you sure that wherever the node type changes there is a corresponding call to $reset?
#14
Please fix code style.
1.) Concatenation
'node_types_'.$language->languagelacks spaces, both when setting and getting cache.2.) Before and after
cache_set('node_types_'..., make sure the blank lines contain no spaces. There are currently two on each.3.) The same applies to both declarations of
global $language#15
Okey, this patch fixes the code style changes.
Benchmarks:
- with apc opcode cache
- enabled modules (Blog, book, comment, DBlog, forum, Help, Locale, Menu, Poll, Simpletest, Taxonomy, Update status)
- dutch d6 PO files for all enabled modules
- no page cache
Command run 'ab -n 500 -c 10 http://drupalhead/'
Without patch:
#1
Requests per second: 52.64 [#/sec] (mean)
Time per request: 189.973 [ms] (mean)
#2
Requests per second: 52.56 [#/sec] (mean)
Time per request: 190.263 [ms] (mean)
#3
Requests per second: 52.59 [#/sec] (mean)
Time per request: 190.133 [ms] (mean)
With patch:
#1
Requests per second: 54.23 [#/sec] (mean)
Time per request: 184.416 [ms] (mean)
#2
Requests per second: 54.51 [#/sec] (mean)
Time per request: 183.441 [ms] (mean)
#3
Requests per second: 54.72 [#/sec] (mean)
Time per request: 182.748 [ms] (mean)
So looks like a small speedgain.. especially when you are using a different memory cache system.
#16
Forgot the patch...
#17
Subscribing...
#18
Unfortunately, my benchmarks don't show such an improvement and with an untranslated interface even a negative effect?
Untranslated/unpatched
Requests per second: 50.19 [#/sec] (mean)
Time per request: 199.245 [ms] (mean)
Requests per second: 49.83 [#/sec] (mean)
Time per request: 200.673 [ms] (mean)
Requests per second: 50.16 [#/sec] (mean)
Time per request: 199.354 [ms] (mean)
Patched:
Requests per second: 48.66 [#/sec] (mean)
Time per request: 205.528 [ms] (mean)
Requests per second: 49.65 [#/sec] (mean)
Time per request: 201.393 [ms] (mean)
Requests per second: 49.76 [#/sec] (mean)
Time per request: 200.972 [ms] (mean)
Translated/unpatched:
Requests per second: 38.34 [#/sec] (mean)
Time per request: 260.837 [ms] (mean)
Requests per second: 39.03 [#/sec] (mean)
Time per request: 256.217 [ms] (mean)
Requests per second: 38.51 [#/sec] (mean)
Time per request: 259.670 [ms] (mean)
Patched:
Requests per second: 38.21 [#/sec] (mean)
Time per request: 261.737 [ms] (mean)
Requests per second: 37.91 [#/sec] (mean)
Time per request: 263.782 [ms] (mean)
Requests per second: 38.07 [#/sec] (mean)
Time per request: 262.702 [ms] (mean)
Benchmark used: 'ab -n 500 -c 10 http://localhost/d7/'
#19
Hmmz back to code needs work then. I'll check it out!
#20
This is somewhat related to #220233: Put node_get_types() out of its misery, as I moved the static cache from node_get_types() to _node_types_build() because that function gets splittet up into multiple, easier to understand functions.
I think the efficiency of this heavily depends on the amount of node types and translations available. If you have only the defaults (Page/Article and only english), caching does not make sense because that actually adds a query (with the default db cache).
Maybe adding a variable (variable_set/get) to configure if the cache should be used ? That could use a default that depends of the number of actived node types with a long description and can be controlled manually in settings.php. (Setting such a default value only makes sense with #145164: DX: Use hook_variable_info to declare variables and defaults, where the defaults are saved in the cache).
#21
Caching of node types has already been implemented based on the current functionality of _node_types_build(). This was committed as a result of #898360: Cache node types.