Faster code for this?

[inexion]

Hello,

I have some code thats taking forever to execute, even with no time limit its painfully slow, and I can't think of a better way of making it quicker, any help much appreciated!

$bcheck = array(
"0" => "NO DATA",
"1" => "VALUE1",
"2" => "VALUE2",
"3" => "VALUE3",
"4" => "VALUE4",
"5" => "VALUE5",
"6" => "VALUE6"
);

$i = 0;
foreach($targets['nid'] as $key => $value){
	$value = $i;
   	$index0 = bindec($fparts[0]);
	$index1 = bindec($fparts[1]);
	$index2 = bindec($fparts[2]);
	$index3 = bindec($fparts[3]);
   	if (($index0 >= 0) && ($index0 <= count($bcheck)))
   	{
       	$rphtsql = "UPDATE targets SET rpht ='".$bcheck[$index0]."' WHERE `targets`.`nid` ='".$targets['nid'][$i]."'";
	mysql_query($rphtsql) or die("query failed".mysql_error());

   	$vphtsql = "UPDATE targets SET vpht ='".$bcheck[$index1]."' WHERE `targets`.`nid` ='".$targets['nid'][$i]."'";
mysql_query($vphtsql) or die("query failed".mysql_error());

   	$bphtsql = "UPDATE targets SET bpht ='".$bcheck[$index2]."' WHERE `targets`.`nid` ='".$targets['nid'][$i]."'";
mysql_query($bphtsql) or die("query failed".mysql_error());

   	$astrsql = "UPDATE targets SET astr ='".$bcheck[$index3]."' WHERE `targets`.`nid` ='".$targets['nid'][$i]."'";
mysql_query($astrsql) or die("query failed".mysql_error());
}

$i++

}

Thanks!

[laserlight]

Well, you could reduce the number of SQL statements executed to a quarter of what you have now by combining the SQL statements in the loop, but if there are many iterations of the loop (and this appears to be the case), this may have limited effect. Without a clearer explanation of the underlying problem you are trying to solve, I am reluctant to give it a go, so the simplest solution I can suggest is to wrap the SQL statements executed in a single transaction, but this may not be possible for you.

The combining of the SQL statements in the loop would look like:

        $sql = "UPDATE targets SET rpht='" . $bcheck[$index0] . "', "
            . "vpht='" . $bcheck[$index1] . "', "
            . "bpht='" . $bcheck[$index2] . "', "
            . "astr='" . $bcheck[$index3] . "' "
            . "WHERE `targets`.`nid`='" . $targets['nid'][$i] . "'";
        mysql_query($sql) or die("query failed".mysql_error());

[inexion]

hey laserlight,

yeah so it basically won't matter how compact the sql is, the iterations here are filling in 4 separate columns which start off blank, and all become filled with one of the values from the bcheck array. Basically whats going on through each iteration is a bitwise check is going on and matching the result to an array value, then filling that value into the table.....overall the time doesn't necessarily matter, but this code was SO slow it couldn't be used.

if I can get this working faster it will be used with large amounts of data (million+ records)

[laserlight]

yeah so it basically won't matter how compact the sql is

The fewer SQL statements are executed, the faster the overall execution is likely to be. Wrapping all the SQL statements in a single transaction helps to eliminate the problem of having many SQL statements executed in the first place, and that is why I suggested it.

Using prepared statements would further eliminate the overhead of having to re-compile the SQL statement for each iteration, but that could well be considered a micro optimisation not worth the trouble since you are using the legacy MySQL extension instead of the MySQLi extension or the PDO extension, both of which have prepared statement support. On the other hand, you might as well switch to the MySQLi extension or PDO extension in the first place, but it would require changes to your existing code.

Basically whats going on through each iteration is a bitwise check is going on and matching the result to an array value, then filling that value into the table

Of course, that is evident from your code. What is more interesting is why you are doing what you are doing, and thus what improvements can be made to the algorithm itself.

[inexion]

yeah i'm by no means any better than a neophyte with this stuff, but i like learning by example etc.. i have some sneaking suspicion theres a way to do this without php altogether but haven't come up with anything better than this.

[laserlight]

i have some sneaking suspicion theres a way to do this without php altogether but haven't come up with anything better than this.

It is possible that PHP is not the best tool for this job, but why not try out my suggestions first and see how it goes?

[inexion]

when i goto work tomorrow ill try it out for sure, thanks laserlight

[mikell]

Been drinking so double check this code, but there's a decent chance it might be faster. Basically a Laserlight rip off with a twist.

<?php
$bcheck = array
(
"0" => "NO DATA",
"1" => "VALUE1",
"2" => "VALUE2",
"3" => "VALUE3",
"4" => "VALUE4",
"5" => "VALUE5",
"6" => "VALUE6"
);

//this doesn't need to be in the while loop. no idea how long the bindec function
//takes to return a value but no need to do it more than once.
$index0 = bindec($fparts[0]);
$index1 = bindec($fparts[1]);
$index2 = bindec($fparts[2]);
$index3 = bindec($fparts[3]);

$i = 0;
$num_targets_nid = count($targets['nid']);

//not using $key or $value so no need for foreach...just use while loop, which 
//would be faster...no idea how much faster though.
while($i < $num_targets_nid)
{
	if (($index0 >= 0) && ($index0 <= count($bcheck)))
	{
		$sql = "UPDATE targets SET rpht='" . $bcheck[$index0] . "', "
            . "vpht='" . $bcheck[$index1] . "', "
            . "bpht='" . $bcheck[$index2] . "', "
            . "astr='" . $bcheck[$index3] . "' "
            . "WHERE `targets`.`nid`='" . $targets['nid'][$i] . "'";

	mysql_query($sql) or die("query failed".mysql_error());
}
$i++
}
?>

[TeraTask]

Are you limited to PHP 4? MySQL 4? If the answer is no to both, then try using multi_query and/or prepare the statements.

Either way, when updating a row by it's unique ID (i.e. there is only 1 row to be updated), add LIMIT 1 to the end of the query.

Mikell points out something important in the code: Loop invariant stuff needs to be outside of a loop. If it doesn't change within the loop, then put it outside the loop.

[Drakla]

If a row has a unique id - and is indexed as such then there's not need to add a limit. If it isn't indexed then you'll get a staggering speed up if you do index it.

[laserlight]

hmm... I noticed an anomaly earlier but it did not occur to me that it is actually a potential bug. Typically, indices range from 0 to one less than the count of the range, or from 1 to the count of the range. However, the test is (($index0 >= 0) && ($index0 <= count($bcheck))). If this check is crucial, then there is a bug, since the check should be (($index0 >= 0) && ($index0 < count($bcheck))).

[TeraTask]

Drakla;10879083 wrote:

If a row has a unique id - and is indexed as such then there's not need to add a limit. If it isn't indexed then you'll get a staggering speed up if you do index it.

I didn't believe this at first. I had read somewhere that it helps. Couldn't find that reference, of course, so I decided to conduct a test and insert 1,000,000 into a table and then searched 2/3 of the way through it with 2 queries, the only difference being LIMIT 1. The results? You're right!

Thanks for teaching me something today. Shows that you can't believe everything you read (or that you need to read it more closely if I misread it.) 🙂

[inexion]

hey guys thanks for the input so far,

all of the $fparts pieces get set inside the loop, and I need all 4 of them for each iteration, so that all needs to be inside unfortunately.

I'll look into trying some of those other changes tomorrow and see what happens =]

[TeraTask]

TeraTask;10879090 wrote:

I didn't believe this at first. I had read somewhere that it helps. Couldn't find that reference, of course, so I decided to conduct a test and insert 1,000,000 into a table and then searched 2/3 of the way through it with 2 queries, the only difference being LIMIT 1. The results? You're right!

Thanks for teaching me something today. Shows that you can't believe everything you read (or that you need to read it more closely if I misread it.) 🙂

Actually just found the code. It was a LIMIT 1 test that I conducted on a non-indexed column. See, I have to pay more attention to what I learn. lol.

Thanks again.

[laserlight]

inexion wrote:

all of the $fparts pieces get set inside the loop, and I need all 4 of them for each iteration, so that all needs to be inside unfortunately.

However, this is not actually in your code. Now do you see why I ask for you to state the bigger picture of what you are trying to do? At the moment we are just suggesting based on your code, but if your code is not the Real Thing, then our suggestions may well be of limited value. It is good to come up with the smallest and simplest (runnable) code that demonstrates the problem, but this example code must demonstrate the problem, not do something else.

TeraTask wrote:

I didn't believe this at first. I had read somewhere that it helps. Couldn't find that reference, of course, so I decided to conduct a test and insert 1,000,000 into a table and then searched 2/3 of the way through it with 2 queries, the only difference being LIMIT 1. The results? You're right!

It makes sense though: there is no reason to continue searching after the result is found when a unique index is used.

[TeraTask]

laserlight;10879095 wrote:

It makes sense though: there is no reason to continue searching after the result is found when a unique index is used.

I know. I thought it was really stupid of the MySQL engine to have a problem with figuring out that there was only one possible solution -- I mean why have the different key types if the key type isn't given meaning?

I'm glad I was wrong.

[inexion]

hmmph, well it seems as if its noticeably faster, but still really slow - slow as in i can refresh my table over and over again and watch the values fill up lol.....

ill explain the whole process.....

inside this foreach loop are a couple other things....

theres a column of hex values that all get converted to binary then split into 5 different parts......a 20-bit string (the end), and 4 separate 3-bit sets (the first 12 bits). these are all used with different masks to determine different aspects of the data.

(if you're wondering, i'm working on making a list of stars in order to apply some very particular criteria for culling from a huge database)

so now, when you saw my code back there - the iteration has to decode those 4 different 3-bit sets, check them against what i store in the array for their text equivalent values, then populate the empty columns in the table with that text.

i'm not sure what part takes so long - i'm almost certain its not the conversion. i have a standby piece of code that prints results to html so i can view how its working out without having to deal with a sql table all the time. i could run that code without a problem normally, filling 20,000 html tables in under 20 seconds with all this data. it seems like its a problem in transit. i will try more testing.

[Drakla]

If you can post more code or something I'd be happy to help with this.

[laserlight]

i'm not sure what part takes so long - i'm almost certain its not the conversion. i have a standby piece of code that prints results to html so i can view how its working out without having to deal with a sql table all the time. i could run that code without a problem normally, filling 20,000 html tables in under 20 seconds with all this data. it seems like its a problem in transit. i will try more testing.

I am reasonably certain that the problem is executing SQL statements in a loop since the conversion is something that computers are pretty fast at. If you cannot use transactions, change to a MySQL table type that allows for them, then wrap those SQL statements in a transaction. Switch to the PDO extension (or the MySQLi extension) and prepare the SQL statement, binding new values in the loop instead of executing the SQL statement from scratch on each iteration.

[Sxooter]

The basic approach you need to take is something along the lines of putting the data you currently feed into your php into a table, then using a query to use that data to update the table you have.

Does that make sense?

Any programmatic approach will not use your database to its fullest.

So, you need to take what's in $targets['nid'] and put it into a table.

Then maybe put $bcheck into a table. Then maybe put bindec($fparts[x]) into a table. Then start figuring out how to run a query that mushes those against each other in a way similar to in your code. Note that I say maybe on these two because they're small enough you can probably build them into where clauses, I don't know your code enough to say.

Do it in SQL. The more you move your big data crunching work into the database, the faster it's gonna be.