doctrine / orientdb-odm

A set of PHP libraries in order to use OrientDB from PHP
http://odino.org/blog/categories/orientdb/
MIT License
155 stars 54 forks source link

improve cache performance (CPU & mem) #156

Closed vicb closed 11 years ago

vicb commented 11 years ago

see https://github.com/doctrine/orientdb-odm/commit/df875c946e02a9c84eee22f0c86e0a3b06bd70ec#commitcomment-2500697

I have preferred to keep unrolled code over factorization to keep perf optimal, let me know if you prefer to have a unique static fromCache().

/cc @odino

odino commented 11 years ago

@nrk @vicb why not having a method called inflectFromCache($inflection, $word) so that there is no duplicate logic?

vicb commented 11 years ago

performance as I've mentioned in the PR comment. If this is a problem, I can update the PR.

odino commented 11 years ago

would this drop performances?

<?php

public static function inflectFromCache($inflection, $word)
{       
        if (!isset(static::$cache[$inflection][$word])) {
            static::$cache[$inflection][$word] = parent::$inflection($word);
         }

        return static::$cache[$inflection][$word];
}
vicb commented 11 years ago

It would as you would have 1 extra function call.

I don't this the proposed changes as an issue if you don't expect to add a lot of cached method. On the contrary if adding a cached method is a common task, your proposal might be good. Up to you guys !

vicb commented 11 years ago

Unless you don't want specific methods but in this cache the API would be different whether you use cache or not... which I don't find the best way to do.

odino commented 11 years ago

I mean significantly drop performances: one more function is not something that will seriously affect performances., no worries

(Like, the cyclomatic complexity of the library is 1.50 :-) in general, I guess, we prefer clean structure over compromises)

So if you are ok with editing the PR and using a base method, that would be great!

odino commented 11 years ago

Also, inflection is done everytime you hydrate a record. If you have 30k records with 6 attributes, its 30x6x1000 inflections :) Thats one of the reasons I integrated the cache earlier, I noticed a good increase of performances through cached inflection.

vicb commented 11 years ago

@odino I think it would significantly impact perf as a function call is a lot compared to what the methods currently do (in term of both CPU and mem as that would mean creating one more frame for each of the calls).

I would not normally unroll code to save perf but it sounds reasonable in a class that only exists to improve perf.

But once again I can do this, please only confirm that you really want this - by reading your last comment I am under the impression that perfs are crucial, which sounds contradictory with your previous comment.

nrk commented 11 years ago

On commodity hardware 100,000 method calls of a static method with 2 arguments using PHP 5.4 (which is faster at method dispatching than previous versions) account for about 20ms worth of overhead so, while I do agree that we should avoid code duplication as a general rule, in the end it really comes down to how much we really want to achieve (micro) optimizations for this part in relation to how often it is going to get used under normal conditions.

To be honest I don't mind the unwinded version of the code commited by @vicb given the nature of this class.

odino commented 11 years ago

then, its in :+1: