CODEWITHSUNDEEP
phpoop
sjosen
sjosen

Reputation: 561

Performance penalty php class

I am developing a web application for my company. The application is used to keep track of the tools we buy (and use).

Each time a worker in the company is using a tool it is registered in the application (mysql db). Until now I have written the code in procedural style. Still a novice in OOP and my procedural code works fine and are relative fast.

Now the application is getting larger and I’m really having issues maintaining my old coding pattern, so I figured it was about time to move on and try implementing OOP. The problem I’m facing now is that I somehow made my classes in a way so the application is up to 10 times slower that before. One of my pages takes 1 sec to load - without the classes it takes 0.1 sec. I use php microtime to measure this.

I’ve created a Company class which holds the properties for each company. Snippet of the class:

class Company {

    private $companyId;
    private $name;
    private $cvr;
    etc…..

    private $tools = array();

}

So far it’s working as expected. Now I wanted to create a method to get all the companytools and attach it to the $tools array.

public function tools() {

    $purchases = $db
    ->table('CompanyTools')
    ->select(
        'id'
    )
    ->where('CompanyTools.companyId', '=', $this->companyId)
    ->get();

    $objects = array();

    foreach ($purchases as $purchase) {

        $objects[] = new CompanyTool($purchase['id']);

    }

    $this->tools = $objects;

    return $this->tools;

}

Now when I use it like this I get the performance issues mentioned ealier

foreach ($company->tools() as $purchase) {

    echo $purchase->id;

}

I suspect it has something to do with the loop creating a new instance of the CompanyTool class. The CompanyTool class looks something like this

class CompanyTool {

    function __construct($id = null) {

    if(!$id) die("We need the id");

    $this->id = $id;

    $attributes = $db
        ->table('CompanyTools as ct')
        ->select(
            'ct.*'
        )
        ->where('ct.id', '=', $this->id)
        ->take(1)
        ->get();

    foreach($attributes as $attribute)
        foreach ($attribute as $key => $value) {
            if(property_exists($this, $key))  $this->{$key} = $value;
        }
    return $this;       


}

I hope it's obvious what I'm trying to achieve and my lack of OOP knowledge :)

Thanks for your input

/ j

Upvotes: 2

Views: 2878

Answers (5)

edigu
edigu

Reputation: 10089

There are lot of things needs to be reviewed in your approach.

Some of them:

  1. Use Namespaces in your classes. Always.
  2. Running a DB query and iterating over the resultset in a constructor is really bad idea.
  3. Running another DB query while iterating over the resultset is worse idea. Probably this is the main performance bottleneck of your app.
  4. You should define a visibility for every properties and methods in you classes.
  5. Basically, it seems like Company and CompanyTool classes are your domain objects (entities. Check out this answer). Entities are simple POPOs and shouldn't aware the databases, queries or any other application logic.
  6. You can start refactoring by de-coupling your database queries from your entities, also you'll need to utilize SQL JOINS to improve performance.
  7. I strongly recommend to read some articles about Object Relation Mappers. You don't need to use them but you have to understand what the real problem is trying to solve ORMs.

Upvotes: 1

KIKO Software
KIKO Software

Reputation: 16688

There is no real clue in the codes you've published to know why your code is slower than before. The only thing I can think of is that you do more MySQL queries in your OOP code than in your procedural code.

When you begin with OOP you might have a tendency to fill your objects completely with data from the database when you create them, even when you're not going to use this data on a particular HTML page. This approach is clearly flawed. A class should only retrieve the data when it is actually needed.

For instance, you have a page that lists the tools, but you show only 25 tools per page. In that case you shouldn't retrieve all tools and put them in a instance of a class, only the 25 that are needed will do.

That was just an example, but I assume you get the idea?

It is not to difficult to check all the MySQL queries you execute. Simply echo all queries that are executed to the screen, or write them to a file, or database, and record how long they took. You will then be able to see where your server spent its time.

But my point is to only access the database when the data is actually needed, and not just to fill objects. That would be pointless.

Upvotes: 1

Halfpint
Halfpint

Reputation: 4079

You definitely spotted your main performance hit, initialising a new constructor each time you pass through the loop is a very bad idea especially as it is running a query each time its built - this is causing a full DB fetch cycle for each object row you are initialising which normally on its own is not computationally expensive but over a larger subset of records will cause a huge performance hit.

A better way of handling this would be to maybe have a single class which encapsulates all tools, on launch it would run a single sql query to pull back every too in the database and store it into an array which you can access, this would only cause one DB fetch cycle.

You could then build some accessor methods to pull out data from the array of tools using specific keywords such as get_product_by_id($prod_id, get_product_by_category($prod_cat) to get the item you need

Some basic pseudo code may look like this - note, you should probably implement this class as a singleton because you only ever want to have one array of CompanyTools available. If you don't know how to do this I'd highly recommend Gooling the Singleton design pattern:

class AllCompanyTools
{
     private $tools; // Array to hold all of your tools

     function __construct()
     {
         $query = SELECT * FROM company_tools ORDER BY product_id DESC
         $res = // Do your execution and capture these tools into an array

         set_results($res); // This method would check the array is not null and then set the locally defned 
     }

     function set_results($results)
     {
         int i = 0;

         // Check size of $res_array, ensure its not null and then load into $tools,
         // make sure $tools is an array of CompanyTools
         .. 

         // Assign each result into the tools array
         foreach($results as $tool)
         {
             // Insert into the index incrementally with the values from the assoc array
             $tools[i] = new CompanyTool($tool[id], $tool[name], $tools[category]);
             i++;
         }
     }

     function get_product_by_id($id) 
     {
          foreach($tools as $tool)
          {
              if($tool->$get_id() == $id)
              {
                 return $tool;
              }
          }
     } 
}

Your CompanyTool class would then be a lot more simplistic

class CompanyTools
{
     // Swap for your vars...
     private $id;
     private $name;
     private $category;

     // Constructor
     function __construct($id_in, $name_in, $cat_in)
     {
        $id = $id_in;
        $name = $name_in;
        $category = $cat_in;
     }

     // Accessors
     function get_name() {
         return self->$name;
     }

     .. Other accessors below
}

Because $tools is an array of CompanyTools we can simply check in the foreach loop whether or not that tool has the same id as the request parameter, if so we return it from the array, instead of connecting to our db processing a transaction, returning the result and then terminating the transaction which is significantly more expensive.

NOTE Please bare in mind the code above is not a definitive solution, you'll need to do some work to build the array of CompanyTools, I just wanted to show how in terms of performance it will benefit you to do one initial fetch request and then use that stored array to get the data you need without having to keep interfacing with your DB.

Upvotes: 0

Ja͢ck
Ja͢ck

Reputation: 173532

You can do a simple rearrangement that should save some cycles:

// get all fields in this query
$purchases = $db
  ->table('CompanyTools')
  ->select('ct.*')
  ->where('CompanyTools.companyId', '=', $this->companyId)
  ->get();

foreach ($purchases as $purchase) {
    // and pass each row to the constructor
    $objects[] = new CompanyTool($purchase);
}

// ====
class CompanyTool 
{
    function __construct(array $attributes) 
    {
        if (!isset($attributes['id'])) die("We need the id");

        // no need to perform any query here
        foreach ($attributes as $key => $value) {
            if (property_exists($this, $key))  $this->{$key} = $value;
        }
    }
}

Upvotes: 2

Jasen
Jasen

Reputation: 12392

Ooh, only 10 times slower, you were lucky!

each time you create a CompanyTool you run a query

$attributes = $db ... that's going to hurt performance

especially since you're using some sort of ORM, that's often good for three times slower all by itself.

If you need all the tools fully instantiated you're going to want to do some sort of join query to fetch all the data and instantiate them in one query, else figure out a way to use placeholders and have them fill in the missing details when they are needed.

Upvotes: 0

Related Questions