4
\$\begingroup\$

I'm not overly efficient when it comes to coding, and I am looking for ways to improve the speed of a log translator I built.

Several files are uploaded to a folder on a server and log files are selected for processing. Speed here is fine. Each log file has 5000-10000 lines possible a bit higher. They are space delimited files that look like this:

20:04:13 + MA 00 61
20:04:17 - MA 00 61
20:04:18 + MA 00 61 optionaltxt
20:04:20 - MA 00 61

There are at least 5 fields for every entry but more can be added if optional text exists. Field 5 is a status code, and it has to be cross referenced with a language file then replaced with the corresponding message so the result is like this:

17:29:48 - MA 00 (061) Feed hold! X / Y axis is locked   
17:29:48 + MA 00 (061) Feed hold! X / Y axis is locked 
17:29:50 - MA 00 (061) Feed hold! X / Y axis is locked optionaltxt
17:29:51 + MA 00 (061) Feed hold! X / Y axis is locked 

The translated logs are saved to a new folder for download later. At the moment translation takes about 7-10 seconds per log file. The problem is that it is common to have 90 files or more and the time starts adding up. I have no control over the log format or language file format.

This part of my code looks like this:

$shLANG = file_get_contents("ErrCODES/data_en.properties");
$SHarray = explode("\n", $shLANG);

$logARRAY = glob($uploadDIR.$filepath."/original/*.[lL][oO][gG]");
foreach ($logARRAY as $logFILEarray) {

$name = basename($logFILEarray);
$logFILE = file_get_contents($uploadDIR.$filepath."/original/".$name);
$logFILEarray = explode("\n", $logFILE);
foreach ($logFILEarray as $key => $line) {
        $newline="";
        $LINEarray = explode(" ", $line);

        if(isset($LINEarray[0])){$newline .= $LINEarray[0]." ";}
        if(isset($LINEarray[1])){$newline .= $LINEarray[1]." ";}
        if(isset($LINEarray[2])){$newline .= $LINEarray[2]." ";}
        if(isset($LINEarray[3])){$newline .= $LINEarray[3]." ";}        
        if(isset($LINEarray[4])){
            foreach ($SHarray as $code) {
                if (strlen($LINEarray[4])>0){
                    if (substr($code, 0, strpos($code, '=')) == $LINEarray[4]) {
                        $newline .= trim(explode('null;', $code)[1]). " ";
                    }
                }
            }           
        }

        if(isset($LINEarray[5])){$newline .= $LINEarray[5]." ";}
        if(isset($LINEarray[6])){$newline .= $LINEarray[6]." ";}
        if(isset($LINEarray[7])){$newline .= $LINEarray[7]." ";}
        if(isset($LINEarray[8])){$newline .= $LINEarray[8]." ";}
        $newline .= "\r";
        $logFILEarray[$key] = $newline;
        }


//var_dump($logFILEarray);
$info = implode("\n",$logFILEarray);
file_put_contents($uploadDIR.$filepath."/translation/".$name, $info);

}//for all glob .log

The code works fine, but how can I improve the speed (greatly)?

\$\endgroup\$
1
  • \$\begingroup\$ I see some stylistic improvements, but, since you asked for speed: 1) have you tried profiling the code to see where it's slow?, 2) instead of file_get_contents and explode, have you considered reading one line at a time (this may actually be slower, though, so just asking, not suggesting). Finally, does it have to be PHP? Unix command line tools may work faster. \$\endgroup\$
    – user1149
    Sep 28 '17 at 12:13
1
\$\begingroup\$

First thing, I would prepare the errorCodes array: After

$SHarray = explode("\n", $shLANG);

Add:

$errors = [];    
foreach($SHArray as $line) {
    list($code, $message) = explode('=', $line);
    $errors[$code] = $message
}

Note: it is not clear from your code what additional processing is needed to get an associative array of $code => $message, you will need to add that yourself. So instead of repeated substring search:

foreach ($SHarray as $code) {
   if (strlen($LINEarray[4])>0){
        if (substr($code, 0, strpos($code, '=')) == $LINEarray[4]) {
            $newline .= trim(explode('null;', $code)[1]). " ";
        }
    }
}   

you will have

foreach($errors as $code => $message) {
    if($code == $LINEarray[4]) { 
        $newline .= $message . " ";
    }
}

Another thing: instead of linear processing of each log field, I would update $lineArray[4], then re-assemble the array using implode, without processing other fields at all, if they do not need to be changed. Note that it may not increase productivity, but will improve code readability and size.

Like this:

$lineArray = explode(' ', $line);
// use a separate function/method to translate one entry
$lineArray[4] = translateErrorMessage($lineArray[4], $errors); 
$newLine = implode(' ', $lineArray);

Finally, With speed and productivity, you need to consider not only your code, but other system factors as well.

  • Which version of php are you using? PHP 7 may give a speed benefit over older 5.xx
  • Are you using standalone php (CLI) or run as a web server? On large files, web server may fail with a timeout, also it is no good to run such tasks with full stack, as it consumes way too much memory.
  • Is it possible to install some PHP pre-compiler with your system? These usually save time, as pre-compiled code works much faster.
\$\endgroup\$
2
  • \$\begingroup\$ Thank you @TimSparrow the associative array was the key part. Now the logs convert in under 1 second, over 10 months can be done as a batch before the timeout which is plenty. It's actually the first time that I've made an associative array so this is very good experience. \$\endgroup\$
    – mochaMame
    Oct 23 '17 at 6:42
  • \$\begingroup\$ @Nama still, for batch processing, I would recommend running it as a background CLI task, so that a timeout would never be an issue. \$\endgroup\$
    – TimSparrow
    Oct 23 '17 at 10:00
3
\$\begingroup\$

What you have looks problematic in a few ways.

  • You have a poor data structure for your localization strings. You have these in an array the must be iterated every single time you are doing a lookup against it. This is an linear complexity (O(n)) operation that when executed inside your loop (another O(n) iteration) give you O(n^2) overall performance. @TimSparrow has touched on a good alternative - to load the localization strings into an associative array the will allow O(1) lookup when being used - but perhaps didn't explain how critical this is to your performance. By building such a data structure up front, you whole algorithm goes to O(m) (to build localization data structure) + O(n) (from your log file loop) instead of O(m * n). To illustrate the impact to your performance, let assume you have a 10K line log file and a 100 line localization file. With your current code, this means you could have up to 1 Million lookup operations you have to perform vs. 10,100 if using a proper data structure.
  • This code is a memory hog. You store every file you are working with wholly in memory, as well as storing the same data in duplicate in the result arrays you are building, not to mention all the temporary array/string conversion you have that, again mean you are going to be holding that information in duplicate in memory. If you can get the memory utilization more optimized here, then you may be more able to do things like for each of the individual log into their own process to parallelize the work.
  • You should consider working with the rows in a more structured manner vs. working with them primarily as strings. fgetcsv(), fputcsv() or similar give you better ways to parse through and write your files.

Incorporating the above thoughts should allow you to greatly streamline both the performance of your script as well as just simplify how it works. You might end up with something like:

$localizations = [];
$handle = fopen('ErrCODES/data_en.properties','r');
// read lines from file one at a time.
while ($line = fgetcsv($handle, 0, ' ')) {
    // this part may vary based on your format
    $localizations[$line[0]] = $line[1];
}
fclose($handle);

$logFiles = glob($uploadDIR.$filepath."/original/*.[lL][oO][gG]");
// here is where at some point you may want to fork processes to paralellize
// now it is serial foreach loop
foreach ($logFiles as $logFile) {
    $translatedFile = str_replace('/original', '/translation/', $logFile); 
    $logHandle = fopen($logFile,'r');
    $translateHandle = fopen($translatedFile, 'w+');
    while($line = fgetcsv($logHandle, 0, ' ') {
        // apply localization
        if(!empty($line[4]) && isset($localization[$line[4]])) {
            $line[4] = $localization[$line[4]];
        }
        fputcsv($translateHandle, $line, ' ');
    }
    fclose($translateHandle);
    fclose($logHandle);
}

A few final thoughts on style: - Your style is a little bit all over the place, which makes your code a littel hard to read. - Particularly variable naming is odd. PHP tends to use a mix of camelCase and snake_case in most code bases you will find (internal PHP functions mix both unfortunately). Your approach to variable syntax is very haphazard, sometimes oddly putting portions of variable names in upper case with apparent cohesive approach, sometimes first letter is uppercase, sometimes it is lower case, etc. - Your indentations are inconsistent. - You could be well-served by better use of spacing around flow control operators. A line of code like if(isset($LINEarray[0])){$newline .= $LINEarray[0]." ";} is extremely hard to read. - Consider familiarizing yourself with the PHP-FIG PHP standards, particularly PSR-1 and PSR-2 which deal with aspects of coding style. These are pretty much the de-facto industry standards.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.