blt04 / doctrine2-nestedset

A NestedSet extension for Doctrine2
GNU Lesser General Public License v2.1
127 stars 49 forks source link

Fetching a tree is pretty slow #3

Closed Drachenkaetzchen closed 14 years ago

Drachenkaetzchen commented 14 years ago

On my development system (Core2 Duo 2.6Ghz) it takes almost 2 seconds to retrieve a tree with 160 nodes (ArrayCache used for development). Do you have an idea what the problem could be? If not, I will see if I find out something later today.

I remember that I had 0.2-0.3 seconds execution time in Doctrine1 nested set with 1k nodes - hopefully we can achieve that performance also!

blt04 commented 14 years ago

On my Core2 2.93Ghz it is taking approximately 220ms for ApcCache, and 235ms for ArrayCache to retrieve a tree with 387 nodes. The actual database queries take less than 1ms.

Honestly, I haven't tested performance too much - though your results do sound too slow. I'm not sure what is happening. Can you try to determine where that time is being spent? Ideally an xdebug cachegrind file, but at least figure out if it is the database queries or the buildTree method.

Drachenkaetzchen commented 14 years ago

Okay, I had multiple issues. First, I didn't notice that I had xdebug turned on - which slows down the whole thing. Then, I forgot to configure the query cache. Now I'm down to 500ms.

However, I stumbled over something else - I turned on logging and noticed that during the fetch of the tree, I receive lots of SQL queries which do query on e.g. >39 and <40. See http://pastebin.com/ekdYysxB

It's noteworthy that the function only seems to query when there are no children. There are absolutely no queries when the category already has children.

What I'm doing is to serialize the whole tree using the function:

public function serializeTree (NodeWrapper $node = null) { if ($node == null) { throw new \Exception("Node must not be null!"); }

    $aData = $node->getNode()->serialize();

    $aData["children"] = array();

    foreach ($node->getChildren() as $child) {
        $aData["children"][] = $this->serializeTree($child);
    }
    return $aData;
}

Is that the way I should do things? If yes, there seems to be a bug.

Drachenkaetzchen commented 14 years ago

I believe that $this->children within NodeWrapper should be null if it is unknown if there are childs, and $this->children should be array() if the children are already fetched, and there are none. How does that sound?

Drachenkaetzchen commented 14 years ago

I changed the function getChildren() of NodeWrapper to read as follows:

public function getChildren() { if($this->children === null) { if ($this->hasChildren()) { $this->children = $this->getDescendants(1); } else { $this->children = array(); } }

    return $this->children;
}

I'm not sure if it works as expected in every case, but I have a pretty decent speed increase now (about 100ms less compared to my previous results, which is 1/5th faster).

blt04 commented 14 years ago

Fixed in 298ec86486253d16944a92b2be067d6b1b495c1c.

Thanks for the bug report and fix! I applied a slightly different fix to getDescendants (matches similar check in getAncestors).

Drachenkaetzchen commented 14 years ago

Perfect, thank you very much!