5
\$\begingroup\$

I'm in the process of reviewing some old code released by an old colleague.

We have a cronjob that executes once in an hour to download some thumbnails: the paths to the thumbnails are stored in a plain array.

// $totalThumbs is usually less than 2000 (each about 3kb)
while ($i < $totalThumbs) {

  $imgName = basename($thumbs_array[$i]);

  $fgc = file_get_contents($thumbs_array[$i]);

  $currentFile = __DIR__ . "/" . $imgName;

  // if file_get_contents doesn't return false
  if ($fgc !== false) {

    // if the file is not in that directory, write the file
    if (!file_exists($currentFile)) {

      file_put_contents($currentFile, $fgc);
      clearstatcache(true, $currentFile);
    
    }
  
  }

  $i++;

  sleep(1);
}

This code works but, for example, we can't use CURL multi_exec because of the limited resources of our server.

Is there a way to improve it (more efficient and/or more secure), considering our hardware limits? We don't need speed, but eventually less memory consumption because the same server is at the same time busy with many other 'jobs'.

Thanks

EDIT (for Mast): one important thing to say is the current idea is to remove this part and use a cronjob to directly store an array in a file, so that the file we are talking about has only to read that array

$dir = "https://mylocaldir";
$thumbs_array = [];

// this one returns JSONP
$raw_json = 'https://endpoint';
$json = file_get_contents($raw_json);

// JSONP removal
$json = str_replace("jsonOutreachFeed(", "", $json);
$json = substr($json, 0, -1);

$decoded_json = json_decode($json);

$itm = $decoded_json->items;

$totalThumbs = count($itm);

for ($i = 0; $i < $totalThumbs; $i++) {
  $thumbs_array[] = $itm[$i]->media->m;
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Can you share the rest of the function as well? \$\endgroup\$
    – Mast
    Commented Jul 23, 2020 at 12:45
  • \$\begingroup\$ @Mast: of course \$\endgroup\$ Commented Jul 23, 2020 at 12:52
  • \$\begingroup\$ When in doubt, our FAQ on asking questions is a great start to make sure a question can be answered efficiently. \$\endgroup\$
    – Mast
    Commented Jul 23, 2020 at 12:54

1 Answer 1

5
\$\begingroup\$

Since the code is only reading the contents of the file to see if there are contents in the file, an optimization for both speed and memory usage would be to use the filesize() function and not read the contents. Reading the contents of the file consumes both time and memory.

Rather than writing the contents read in, copy or move the original file to the new location using either rename() or move_uploaded_file().

\$\endgroup\$
4
  • \$\begingroup\$ So, correct me if I'm wrong: you are suggesting not to use file_get_contents and use filesize() instead. And you are suggesting to remove file_put_contents and use copy instead. Am I understanding well? \$\endgroup\$ Commented Jul 23, 2020 at 13:15
  • \$\begingroup\$ That is what I am suggesting, yes. \$\endgroup\$
    – pacmaninbw
    Commented Jul 23, 2020 at 13:24
  • \$\begingroup\$ I must try, it looks like a very promising approach, thanks! :-) \$\endgroup\$ Commented Jul 23, 2020 at 13:25
  • 1
    \$\begingroup\$ Your idea was great but it didn't work for techical reasons: we couldn't read the filesize of the remote file. I tried to replace it with get_headers (HEAD context), but I wasn't lucky. Don't worry, maybe they'll listen to me the next time I say we need better hardware. :-) \$\endgroup\$ Commented Jul 24, 2020 at 8:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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