modules block speed enhancement

testy1 · Involved Joined: Apr 06, 2008 Posts: 483

Be good if someone else could verify this but after changing the following code, The modules block picked up nearly a second

Original:

Code:

/* If the module doesn't exist, it will be removed from the database automatically */
$result2 = $db->sql_query('SELECT title FROM ' . $prefix . '_modules');
while ($row2 = $db->sql_fetchrow($result2)) {
$title = stripslashes($row2['title']);
$a = 0;
$handle=opendir('modules');
while ($file = readdir($handle)) {
if ($file == $title) {
$a = 1;
}
}
closedir($handle);
if ($a == 0) {
$db->sql_query('DELETE FROM '.$prefix.'_modules WHERE title=\''.$title.'\'');
}
}

Modified:

Code:

/* If the module doesn't exist, it will be removed from the database automatically */
$result2 = $db->sql_query('SELECT title FROM ' . $prefix . '_modules');
while ($row2 = $db->sql_fetchrow($result2)) {
$title = stripslashes($row2['title']);
$a = 0;
$handle=opendir('modules');
while ($file = readdir($handle)) {
if (is_dir($file) && $file !== '.' || $file !== '..' ) {
if ($file == $title) {
$a = 1;
}
}
}
closedir($handle);
if ($a == 0) {
$db->sql_query('DELETE FROM '.$prefix.'_modules WHERE title=\''.$title.'\'');
}
}

EDIT: It has added another cost for is_dir() Sad

fkelly · Posted: Tue Mar 31, 2009 7:55 am

How are you measuring when you say you picked up a second? Intuitively I don't see where adding another layer of tests (the if is_dir statement) is going to be more efficient but of course the poof is in the prudding.

Raven · Posted: Tue Mar 31, 2009 3:43 pm

It really says while ($file = readdir($handle)) { ? That will always resolve to TRUE!

Correction: See the posts on down the way to get the entire resolution.

Instead of reading while ($file = readdir($handle)) { or while ($file == readdir($handle)) { it should read while (false !== ($file = readdir($handle))) { . It is actually an assignment and not a logical comparison.

testy1 · Involved Joined: Apr 06, 2008 Posts: 483

yes I realized that it dropped a second or .8 of a second but is_dir incurred a cost of 2.8 ROTFL

I was so excited I just posted lol

Raven · Posted: Wed Apr 01, 2009 10:12 am

We need to enter a Mantis issue on the logic error - = vs ==. You can't just change it because I tried that yesterday and that shut down all my modules to inactive so that logic block needs to be reworked.

Guardian2003 · Posted: Wed Apr 01, 2009 10:23 am

Hmm, I don't even see the need for half of that stuff in the modules 'block'.
Access permissions are updated through admin functions so all the block needs to do is read from the DB once, how many modules are available.
I don't see the need for reading the DIR at all.

fkelly · Posted: Wed Apr 01, 2009 11:26 am

Guardian ... I think the logic may have been: what if someone goes into the file system and deletes /modules/my_module ... I mean the whole directory. The MYSQL table for modules would still have a record for my_module. So, we want to delete that record before the system tries to access the module file and creates an error.

Granted anyone with file system access should know enough to use Modules administration first and delete the module there before removing it from the file system but I'm not sure that always happens that way. I'm not sure this function needs to be run on every page load for every user but that's another matter.

testy1 · Involved Joined: Apr 06, 2008 Posts: 483

Just as a quick test I did a complete fresh install and did a debug run with the following settings;

Not logged in as admin
Not logged in as a user (In other words I was just a guest)
Each test was run using ctr-F5 in firefox which is suppoose to clear the cache
Each test was run on the home page (index.php)
I ran each test 10 times and calculated the average
My wife over looked the whole procedure because she thinks she knows everything (I haven't got the heart to tell her otherwise )

1. With the above in mind and all blocks active
189 functions called in an Average of 330 milliseconds
Actual cachefile size: 407KB

2. With the above in mind and all blocks active except the modules block
184 functions called in an Average of 263 milliseconds
Actual cachefile size: 271KB

It does not look like much, but it is actually a 79% gain.

anyway take it as you will.

fkelly · Posted: Wed Apr 01, 2009 5:54 pm

This just goes to show how complex and difficult and subject to interpretation performance measurement can be. You could probably find some other block to deactivate that would yield an even greater performance gain. And your last post really doesn't address the proposed changes you made in your first post about the modules block and alternative ways of coding it.

I don't say that negatively. The Nusphere Phped product includes a profiler that lets you see the relative "performance" of every PHP statement. Generally we are going to get the biggest performance increases (improvements) by reducing traffic between client and server ... for instance caching css and javascript ... and by reducing both database and file system IO. After that we can sometimes find particular PHP statements that are "pigs" and find alternatives for them.

Guardian2003 · Posted: Thu Apr 02, 2009 2:25 am

Al good points!
I'm still on the fence about the DIR read though I can see now why it is there. I guess it depends if it falls over gracefully in the scenarion that the module is listed in the DB but the directory is actually not there.

In any event, the current methodology is far superior to what standard nuke had - there were several functions doing this at every page load, at least now there is only the one.

testy1 · Involved Joined: Apr 06, 2008 Posts: 483

fkelly wrote:

You could probably find some other block to deactivate that would yield an even greater performance gain.

The modules block is the only standard block with RN that is what I consider a bottleneck, Although It's only a bottleneck when you compare it to the rest of the blocks.

Overall it is probably not that bad, but compared to the other blocks its very naughty Smile

EDIT: suppose we need to remember, we are talking in milliseconds lol

Raven · Posted: Thu Apr 02, 2009 7:09 am

The immediate issue is to correct the logic error I mentioned above.

fkelly · Posted: Thu Apr 02, 2009 7:14 am

Actually, on a couple of sites that I support, the administrator (who is not a programmer by any means) has deactivated the modules block and used the wysiwyg editor to replace it with a better looking (larger text, highlights etc.) set of links to "areas" she wants to accent. If you are not going to be swapping modules in and out willy-nilly and you are only using a subset of the standard Nuke modules and blocks, this approach makes a lot of sense. But of course that's not relevant to your initial discussion of the performance of the standard RN modules block.

testy1 · Involved Joined: Apr 06, 2008 Posts: 483

Raven wrote:

The immediate issue is to correct the logic error I mentioned above.

argh, ok I get it now, I thought you meant the changes you made didn't work.So there is an actual problem with current module block.

Raven · Posted: Thu Apr 02, 2009 4:56 pm

Raven wrote:

It really says while ($file = readdir($handle)) { ? That will always resolve to TRUE! It should read while ($file == readdir($handle)) {

testy1 · Involved Joined: Apr 06, 2008 Posts: 483

I changed it according to the php site and it seem to work alright

Code:

while (false !== ($file = readdir($handle))) {
if ($file != "." && $file != "..") {
//while ($file = readdir($handle)) {
if ($file == $title) {
$a = 1;
}
}
}

duck · Involved Joined: Jul 03, 2006 Posts: 267

Raven wrote:

It really says while ($file = readdir($handle)) { ? That will always resolve to TRUE! It should read while ($file == readdir($handle)) {

I think they are ignoring you Raven? Laughing

evaders99 · Posted: Thu Apr 02, 2009 7:30 pm

Actually Raven, readdir is using a directory resource. It isn't a logical variable comparison, rather its the transaction of putting data into $file that is being used to continue the loop.

readdir will continue to return data from the file handle until it is exhausted. It will return false when there is nothing left. The single = logic is completely correct here.

Only registered users can see links on this board!
Get registered or login to the forums!

Raven · Posted: Thu Apr 02, 2009 10:58 pm

Evaders,

tetsy1 has the correct way and that makes sense. If you read the link you posted you will see why I was complaining Smile

. The way it's written in block-modules.php is wrong.

php.net states:

/* This is the correct way to loop over the directory. */
while (false !== ($file = readdir($handle))) {
echo "$file\n";
}

/* This is the WRONG way to loop over the directory. */
while ($file = readdir($handle)) {
echo "$file\n";
}

evaders99 · Posted: Thu Apr 02, 2009 11:05 pm

I understand, it's not the same as what you posted though. (for duck's sake) I don't think anyone is being ignored Smile

Code:

while ($file == readdir($handle)) {

This is completely not correct, for those viewing this post - just have to be careful where you're putting singe = and double == operations:)

Raven · Posted: Thu Apr 02, 2009 11:17 pm

Good point! I saw the error but in my haste I got locked on the = vs. ==. I will modify my post above. I have also entered a Mantis issue.