8
\$\begingroup\$

I am checking over 160,000 records in my database every 24 hours to see if there were changes in the incoming data coming from API and my existing data in the database. If there were I am updating those, otherwise I'm doing nothing. Checking 189 days takes 10 seconds and 160,000 about 2.5 hours.

What can I improve in my code to speed this up?

$listings = Array('23169','23567','23114','5487'); //Rooms to check (189 days in each room)
$count_listings = count(listings); //count the rooms
$continue = TRUE;
$x = 0;

while ($continue == TRUE && $x < $count_listings) {
    foreach ($listings as $list) {
        //Generates a link from the looped @list
        $url = $this->generate_link($list);
        $results_page = $this->http_get_contents($url, $this->generate_proxy()); //JSON output
        $calendar_month = json_decode($results_page, true); //Into Array

        $data = Array();

        foreach ($calendar_month['calendar_months'] as $month) {
            foreach ($month['days'] as $day) {
                $data[] = $day; //Getting each day
            }
        }
        //Super_unique removes duplicate days
        foreach ($this->super_unique($data) as $day_info) {
            $data_listing = Array(
                'list_id'           => $list,
                'list_price'        => $day_info['price']['local_price'],
                'list_currency'     => $day_info['price']['local_currency'],
                'list_date'         => $day_info['date'],
                'list_available'    => $this->check_list_available((int)$day_info['available'])
            );
            //First get incoming result and search in the database
            $check = $this->db->select('list_price, list_available')
                ->from('airbnb_lists_price')
                ->where("(list_id = '{$data_listing['list_id']}' and list_date = '{$data_listing['list_date']}')")
                ->get();
            //If the record exists 
            if ($check->num_rows() > 0) {
                foreach ($check->result() as $row) {
                    //If the record info was changed update
                    if ($row->list_price != $data_listing['list_price'] || $row->list_available != $data_listing['list_available']) {
                        $this->db->where("(list_id = '{$data_listing['list_id']}' and list_date = '{$data_listing['list_date']}')");
                        $this->db->update('airbnb_lists_price', $data_listing);
                    }
                    else {
                    //do nothing
                    }
                }
            }
        }
        $x++;
    }
    if ($x > $count_listings) {
        $continue = FALSE;
    }
}
\$\endgroup\$
4
  • 4
    \$\begingroup\$ Is there a specific constraint that forces you to do database calls in a loop in your PHP code? This would likely be many times faster if you just handled it on the database side using a stored procedure... \$\endgroup\$
    – Phrancis
    Commented Dec 9, 2015 at 15:33
  • \$\begingroup\$ $this->http_get_contents is likely to be not very fast. \$\endgroup\$
    – njzk2
    Commented Dec 9, 2015 at 20:55
  • \$\begingroup\$ any reason you can't just truncate the table and replace it with what comes back from the api instead of checking for changes? \$\endgroup\$ Commented Dec 9, 2015 at 22:47
  • \$\begingroup\$ The first thing I always look at is the number of nested loops. Normally, anything over O(n)^2 screems inefficient. en.wikipedia.org/wiki/Big_O_notation \$\endgroup\$ Commented Dec 9, 2015 at 23:14

4 Answers 4

12
\$\begingroup\$

There is multiple things that can be optimized in this code.

It is pretty obvious that most time is spent in HTTP requests to remote API and in database queries, so these points should be first in list of optimizations.

Your code doesn't tells much about what is coming from remote API and you didn't tell if you really need to run all requests for this API each day. If amount of requests for remote API can be somehow reduced - you should do it. If not - you need to think about how "fresh" information from this API you need to have, maybe part of information can be cached and re-used.

Besides this, if you need information from remote API on daily basis - you may think about refactoring API requests loop into separate service that will run during whole day and store received information in some local data storage (e.g. database or some cache). Then your primary script will not need to spend time waiting for remote API responses and will run much faster.

If you know that significant amount of information that came from remote API updates less often than daily and you need to process only information that really changed - you should think about hashing. For example you can query API (even in this loop), clean it up from any information that may change at every request (e.g. request date) and, calculate hash from it (e.g. sha1) and store pair (url=>hash) somewhere. Then on the next iteration of script run you will be able to compare hash from new API response with already available hash and if they're same - completely skip data comparison with database.

About database queries - you should check that your query uses database indexes, it may significantly improve query performance.

And less intensive, but still useful optimizations is to avoid repetitive creation of things that you need to create once. For example:

  • Do you need to call generate_proxy() at each request?
  • You should use prepared SQL queries if your library supports them
  • You may collect information that should be updated into database into local array and then send it into database as a single transaction at the end of data processing - it may be faster.
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Nice first answer! Welcome to Code Review! \$\endgroup\$
    – Phrancis
    Commented Dec 9, 2015 at 18:46
3
\$\begingroup\$

This might be classified as a micro-optimization but...

As handy as the Query Builder is, it runs quite a lot of code putting together a query statement.

So instead of $this->db->select(...)->from(...)->where(...)->get();

Write out the query in a more direct manner.

//First get incoming result and search in the database
$sql = "SELECT list_price, list_available FROM airbnb_lists_price WHERE list_id = ? AND list_date = ?";

$check = $this->db->query($sql, array($data_listing['list_id'], $data_listing['list_date']);
//If the record exists 
if($check->num_rows() > 0){
   ...

The above is essentially what Query Builder will run - eventually. You get the same outcome with a lot less code execution with the more direct approach.

\$\endgroup\$
2
\$\begingroup\$

Cumbersome looping

Let's just look at the two outer loops:

$continue = TRUE;
$x = 0;
while ($continue == TRUE && $x < $count_listings) {
    foreach ($listings as $list) {
        …
        $x++;
    }
    if ($x > $count_listing) {
        $continue = FALSE;
    }
}

Isn't that just equivalent to this simple loop?

foreach ($listings as $list) {
    …
}

Furthermore $list is not actually a list, but a listing or a room number. Renaming the variable would make the code less confusing.

foreach ($listings as $room_number) {
    …
}

Mysterious deduplication

What's $this->super_unique($data)? Your comment says that it removes duplicate keys, but I'm still confused. Is it because $data contains information for days in multiple calendar months? Why would that even make sense? A better explanatory comment is called for.

Better strategy

In the end, you are essentially doing

SELECT list_price, list_available
    FROM airbnb_lists_price
    WHERE list_id = ? AND list_date = ?;

and possibly also

UPDATE airbnb_lists_price
    SET list_id = ?
      , list_price = ?
      , list_currency = ?
      , list_date = ?
      , list_available = ?
    WHERE list_id = ? and list_date = ?

for each listing and each date.

The problem is, that's a lot of SELECT and UPDATE queries, and each one has a significant round-trip overhead.

A better strategy would be to unconditionally bulk upload the entire data set to a new temporary table, then do just one query:

UPDATE airbnb_lists_price p
    JOIN api_results_temp t
        ON p.list_id = t.list_id AND
           p.list_date = t.list_date
    SET p.list_price = t.list_price
      , p.list_currency = t.list_currency
      , p.list_available = t.list_available
\$\endgroup\$
0
\$\begingroup\$

I do not know exactly why your code is slow but I highly recommend that you start using a few basic debugging techniques:

Try this:

$start = microtime(TRUE);

$results_page = $this->http_get_contents($url, $this->generate_proxy());

echo (microtime(TRUE) - $start).' seconds<br>';

Based on your loops, this code should output something like:

0.435436 seconds
1.757436 seconds
0.234559 seconds
2.934545 seconds

You should bounce that microtime(TRUE); to a few suspect locations until you can pinpoint exactly what takes an inordinate amount of time.


My personal guess and recommendation is that you should truncate and populate an identical temp table with the API data and then use just one SQL query to update your live table with the changes found in the temp table.

update
    live_tbl l
    inner join temp_tbl t on l.id = t.id
set
    l.field1 = t.field1
    ,l.field2 = t.field2
    ,l.field3 = t.field3
    ,l.field4 = t.field4
where
    ifnull(l.field1, '') <> ifnull(t.field1, '')
    or ifnull(l.field2, '') <> ifnull(t.field2, '')
    or ifnull(l.field3, '') <> ifnull(t.field3, '')
    or ifnull(l.field4, '') <> ifnull(t.field4, '')
\$\endgroup\$

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.