7
\$\begingroup\$

I'm using the rolling curl library to fire HTTP requests for content-length headers of images (we need to know their size to weed out placeholders and low res images). The image URLs are stored in a database so I need to loop over the data in our products table (approx 1 million rows but will grow bigger, potentially much bigger).

I'm using PHP and the Laravel framework (the artisan CLI component). The operation seems to slow down as time progresses e.g. it starts processing 100 requests in less than a second and later the time to process 100 rows/requests is logged at over 20 seconds. Can anyone explain this and / or offer any performance improvement suggestions? The task is running on an Amazon EC2 micro instance so processing power / memory is limited.

public function fire()
{
    $dt = new DateTime();
    Log::info("started: ".$dt->format('Y-m-d H:i:s'));
    $counter = 1;
    Item2::where('img_size', '=', NULL)->chunk(1000, function($items) use ( &$counter)
    {
        $results = array();
        $filePath = storage_path().'/imports/new/new_img_sizes_'.$counter.'.csv';

        if (!File::exists($filePath)) {
            File::put($filePath, '');
        }
        $start = microtime(true);
        $rollingCurl = new \RollingCurl\RollingCurl();
        $rollingCurl->setOptions($this->curlOptions);

        foreach ($items as $item)
        {
            if ($item->img !== '') {

                $results[$item->id] = array('url' => $item->img, 'size' => null);
                $rollingCurl->get($item->img);
            }

        }
        //callback runs on each curl request
        $rollingCurl->setCallback(function(\RollingCurl\Request $request, \RollingCurl\RollingCurl $rollingCurl) use (&$results, $filePath) {
            $responseInfo = $request->getResponseInfo();
            //var_dump($responseInfo);exit;
            $length = $responseInfo['download_content_length'];

            foreach ($results as $key => $value) {
                if (array_search($request->getURL(),$value)) {
                    $idKey = $key;
                    $results[$idKey]['size'] = $length;
                    File::append($filePath,$idKey.','.$results[$idKey]['size']."\r\n");
                    break;
                }
            }

        })

            ->setSimultaneousLimit(10)
            ->execute();

        $counter++;
        echo 'done in...'.(microtime(true) - $start).PHP_EOL;
        Log::info('1000 records: '.(microtime(true) - $start));
        Log::info('Last url was: '.json_encode(end($results)));
        exit;
    }); // end item chunk

Some benchmarks:

done in...3.8803641796112
done in...7.4326379299164
done in...8.1860301494598
done in...8.5088090896606
done in...10.606615781784
done in...10.655412912369
done in...10.804574966431
done in...14.004528045654
done in...10.903785943985
done in...11.905344009399
done in...13.763195991516
done in...14.723680019379
done in...15.823812961578
done in...17.972007989883
done in...31.734715938568
done in...20.509822845459
done in...22.924754858017
done in...34.274693012238
done in...39.217702865601
done in...29.883662939072
done in...24.094554901123
done in...25.726534128189
done in...31.788655996323
done in...24.713880062103
done in...25.855134963989
done in...23.161122083664
done in...32.380167007446
done in...36.53077507019
done in...31.859884023666
done in...71.458341121674
\$\endgroup\$
  • \$\begingroup\$ Not to nitpick, but you have a very odd, if non-existent, coding style. \$\endgroup\$ – jsanc623 Jan 30 '14 at 16:27
  • 1
    \$\begingroup\$ it's a very rough prototype. which has been hacked around while I get something working. Not sure it is 'nitpick'ing but it is certainly not constructive \$\endgroup\$ – codecowboy Jan 30 '14 at 16:44
  • \$\begingroup\$ I do apologize for lack of constructiveness, its just my pet peeve :) I actually don't see much you can do here, perhaps someone else can. Have you tried logging or lapping throughout the function to see if you're getting rate limited on the service side? Look up (github.com/jsanc623/PHPBenchTime) and use or copy the lapping function there to knock out rate limiting? \$\endgroup\$ – jsanc623 Jan 30 '14 at 16:50
  • \$\begingroup\$ Actually, looking at it again - perhaps lower the concurrency that you set the second time from 100 (setSimultaneousLimit(100)) to 10 or less and see if this gives you better performance. \$\endgroup\$ – jsanc623 Jan 30 '14 at 16:54
  • \$\begingroup\$ @jsanc623 How would the lapping function 'knock out' rate limiting? You mean rule it out as a source of the performance problem? \$\endgroup\$ – codecowboy Feb 5 '14 at 15:03
5
+25
\$\begingroup\$

I don't see what could be causing ever-decreasing performance since the only state outside the main loop is a counter, but adding calls to memory_get_usage as @jsanc623 recommends would help with that.

But you can definitely improve the performance by avoiding the loop over $results for every URL returned. Take advantage of PHP's array which is a hash table providing O(1) lookup.

First, make the URL the key for the results array.

foreach ($items as $item) {
    if ($item->img !== '') {
        $url = $item->img;
        $results[$url] = array(
            'id' => $item->id, 
            'url' => $url,
            'size' => null
        );
        $rollingCurl->get($url);
    }
}

Next, use isset instead of array_search and looping.

// foreach ($results as $key => $value) { ... } becomes

$url = $request->getURL();
if (isset($results[$url]) {
    $results[$url]['size'] = $length;
    File::append($filePath, $results[$url]['id'] . ',' . $length . "\r\n");
}

Since you're placing each URL into the array before making the GET request, the if isn't even necessary.

Separately, you can delay writing to the file until the end of the inner loop so you write one big chunk rather than one thousand tiny chunks. It may be that I/O is more costly, and perhaps you're getting killed by seeking to the end of an ever-growing file? Seems unlikely but maybe after a million writes it adds up.

General Review

There are a few things that could be cleaned up here as well.

  • Consistent formatting: put your opening brace on the same line or the next, but do it consistently.
  • Blank lines before the closing brace seem misplaced to me. Use blank lines to logically group related code, though functions are preferred to whitespace.
  • Simplify testing if the image URL exists

    if ($item->img !== '')
    

    can be simplified to

    if ($item->img)
    

    to skip null values as well. Even if the field cannot be null, it's still easier to read.

\$\endgroup\$
4
\$\begingroup\$

Be sure to disable Laravel's query logging and filter all unwanted results directly with your SQL query. Also use a stream to write lots of small chunks of data, it's much more efficient (and what streams where made for).

Here's a possible solution, would love to see the benchmarking results for this one.

<?php

class CodeReview {

  /**
   * Keep track of the Laravel chunk we're processing.
   *
   * @var integer
   */
  protected $counter = 1;

  /**
   * File handle of the current CSV file (writing only).
   *
   * @var resource
   */
  protected $fh;

  /**
   * Array to map image URLs to IDs.
   *
   * @var array
   */
  protected $urlToId = array();

  /**
   * Used for caching only.
   *
   * @var string
   */
  protected $storagePath;

  /**
   * ...
   *
   * @return this
   */
  public function fire() {
    // Directly format current time and embed variable.
    $date = \DateTime::format("Y-m-d H:i:s");
    Log::info("Started: {$date}");

    // Get storage path once (why is this even a function?).
    $this->storagePath = storage_path();

    // Disable Laravel's query log, otherwise it will eat up our RAM.
    //
    // @link http://laravel.com/docs/database#query-logging
    DB::connection()->disableQueryLog();

    // Execute the query and filter all unwanted results right away. Don't use closures because they are slower than
    // native methods.
    Item2::whereNotNull("img_size")->where("img_size", "<>", "''")->chunk(1000, array($this, "chunkedWhereCallback"));

    return $this;
  }

  /**
   * Callback for the Laravel chunked WHERE query method.
   *
   * @internal Must be public in order to be accessible for PHP's call_user_func() function.
   * @param array $results
   *   The chunked results from the Laravel WHERE query.
   * @return this
   */
  public function chunkedWhereCallback($results) {
    // Open file handle to CSV file.
    $this->fh = fopen("{$this->storagePath}/imports/new/new_img_sizes_{$this->counter}.csv", "w");

    // Instantiate new rolling cURL and configure it.
    $rollingCurl = new \RollingCurl\RollingCurl();
    $rollingCurl->setOptions($this->curlOptions);

    // Micro benchmark
    $start = microtime(true);

    /* @var $result \Namespace\Of\Stub\Class\ClassName */
    foreach ($results as $result) {
      // Build hash array for easy lookup.
      $this->urlToId[$result->img] = $result->id;

      // Add this image to the cURL queue.
      $rollingCurl->get($result->img);
    }

    // That's it, we added all images to the array, let's fetch them. Again we use a native method and no closure.
    $rollingCurl->setCallback(array($this, "rollingCurlCallback"))->setSimultaneousLimit(10)->execute();

    // Finished handling chunk, close CSV handle and increase counter.
    fclose($this->fh);
    ++$this->counter;

    $end = microtime(true) - $start;
    Log::info("1000 records: {$end}");
    $last = end($this->urlToId);
    Log::info("Last URL was: {$last}");

    return $this;
  }

  /**
   * Write image size to CSV file.
   *
   * @param \RollingCurl\Request $request
   *   The rolling curl request.
   * @return this
   */
  public function rollingCurlCallback($request) {
    $size = $request->getResponseInfo()["download_content_length"];
    $url  = $request->getURL();

    if (fwrite($this->fh, "{$this->urlToId[$url]},{$size}\r\n") === false) {
      throw new \RuntimeException("Couldn't write to CSV file!");
    }

    return $this;
  }

}
  • Format your code in a consistent fashion (stick to PSR-2 rules if you're unsure)
  • Try to document your code where it makes sense for you, or for a developer that might have a look at it later
  • Closures hurt readability, maintainability and performance (some may disagree with this, but in your case they definitely do because they are too long)
  • Return $this by default if you have no return value (like in jQuery) and allow chaining of all your methods by default
  • Forget about type checks if performance is all you want (they only add overhead); but don't forget that they are great for public APIs and other stuff (like you get it if you use a framework), it makes things easier for other developers
\$\endgroup\$
  • \$\begingroup\$ your setCallBack fatals with Catchable fatal error: Argument 1 passed to RollingCurl\RollingCurl::setCallback() must be an instance of Closure, array given \$\endgroup\$ – codecowboy Mar 12 '14 at 15:12
  • \$\begingroup\$ Never actually tried the code, how could I, but this means the RollingCurl has a really, really poor implementation of it's callbacks because it seems like it's checking the instance instead of is_callable() and using e.g. call_user_func(). \$\endgroup\$ – Fleshgrinder Mar 12 '14 at 15:48
2
\$\begingroup\$

Give this a try and post the results - it integrates PHPBenchTime laps and get_memory_usage() with your script.

require('PHPBenchTime.php');
use PHPBenchTime\Timer as Timer;

public function fire(){
    $Benchmark = new Timer;
    $Benchmark->Start();

    $dt = new DateTime();
    Log::info("started: ".$dt->format('Y-m-d H:i:s'));
    $counter = 1;

    Item2::where('img_size', '=', NULL)->chunk(1000, function($items) use ( &$counter){
        $results = array();
        $filePath = storage_path().'/imports/new/new_img_sizes_'.$counter.'.csv';

        if (!File::exists($filePath)) {
            File::put($filePath, '');
        }
        $rollingCurl = new \RollingCurl\RollingCurl();
        $rollingCurl->setOptions($this->curlOptions);

        foreach ($items as $item){
            if ($item->img !== '') {
                $results[$item->id] = array('url' => $item->img, 'size' => null);
                $rollingCurl->get($item->img);
            }
        }

        //callback runs on each curl request
        $rollingCurl->setCallback(function(\RollingCurl\Request $request, \RollingCurl\RollingCurl $rollingCurl) use (&$results, $filePath) {
            $responseInfo = $request->getResponseInfo();
            //var_dump($responseInfo);exit;
            $length = $responseInfo['download_content_length'];

            foreach ($results as $key => $value) {
                if (array_search($request->getURL(),$value)) {
                    $idKey = $key;
                    $results[$idKey]['size'] = $length;
                    File::append($filePath,$idKey.','.$results[$idKey]['size']."\r\n");
                    break;
                }
            }
        })

            ->setSimultaneousLimit(10)
            ->execute();

        Log::info('Last url was: '.json_encode(end($results)));

        $Benchmark->Lap("Lap " . $counter . " mem " . memory_get_usage() . " bytes");

        $counter++;

        exit;
    }); // end item chunk


    $time = $Benchmark->End();
    print_r($time);
}
\$\endgroup\$
  • \$\begingroup\$ While this is probably a valuable answer, it does not provide a review of the OP's code. Please add more details, explain how this solution is different/better than the original code. \$\endgroup\$ – Mathieu Guindon Feb 7 '14 at 19:02
  • \$\begingroup\$ @lol.upvote Wasn't really meant as a full answer - rather just integrated memory_get_usage and a timer. I'd rather the code reside here than in a gist or a pastebin. \$\endgroup\$ – jsanc623 Feb 7 '14 at 19:12
  • 1
    \$\begingroup\$ I'd rather the code reside here than in a gist or a pastebin. - and that's a good call, as a link-only answer would have been much worse. However code-only answers don't rank much higher. Feel free to edit whenever you have a chance :) \$\endgroup\$ – Mathieu Guindon Feb 7 '14 at 19:16
  • \$\begingroup\$ Please see this meta question/answers for more info. \$\endgroup\$ – Mathieu Guindon Feb 7 '14 at 19:22
  • \$\begingroup\$ Not to get in a spitting match here, but do note that I was commenting in codecowboy's question and this edited version of the code is requesting more information as I don't have access to codecowboy's environment. But if you'd like, I can make it a gist and just link it in a comment in their question. \$\endgroup\$ – jsanc623 Feb 7 '14 at 19:37

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.