On Sat, May 28, 2011 at 04:17:09PM -0700, elij wrote:

- move apc cache code to aur.inc (centralize) - refactor the apc usage in stats.inc to utilize new code in aur.inc --- web/lib/aur.inc | 49 ++++++++++++++++++++++++++++++++++++++++++ web/lib/stats.inc | 61 +++++++++++----------------------------------------- 2 files changed, 62 insertions(+), 48 deletions(-)

diff --git a/web/lib/aur.inc b/web/lib/aur.inc index fb267af..2b78c99 100644 --- a/web/lib/aur.inc +++ b/web/lib/aur.inc @@ -14,6 +14,12 @@ include_once("config.inc"); include_once("version.inc"); include_once("acctfuncs.inc");

+# Check if APC extension is loaded, and set cache prefix if it is +if (!defined('EXTENSION_LOADED_APC')) { + define('EXTENSION_LOADED_APC', extension_loaded('apc')); + define('APC_PREFIX', 'aur:'); +} + # see if the visitor is already logged in # function check_sid() { @@ -257,6 +263,49 @@ function db_query($query="", $db_handle="") { return $result; }

+# set a value in the cache (currently apc) if cache +# is available for use. if not available, this becomes +# effectively a no-op (return value is false) +# accepts an optional TTL (defaults to 600 seconds) +function set_cache_value($key, $value, $ttl=600) { + $status = false; + if (EXTENSION_LOADED_APC) { + $status = apc_store(APC_PREFIX.$key, $value, $ttl); + } + return $status; +} + +# get a value from the cache (currently apc) if cache +# is available for use. if not available, this +# returns false (optionally sets passed in variable $status +# to false, much like apc_fetch behaves). this allows +# for testing the fetch result appropriately even in the event +# that a 'false' value was the value in the cache. +function get_cache_value($key, &$status=false) { + if(EXTENSION_LOADED_APC) { + $ret = apc_fetch(APC_PREFIX.$key, $status); + if ($status) { + return $ret; + } + } + return $status; +}

I'd prefer to change get_cache_value()'s signature to return the status indicator and pass the actual result by reference. That way, it could be used as follows: ---- if (get_cache_value('foo', $foo)) { do_something $foo } ---- That just feels much more common and convenient. Any objections?

+ +# run a simple db query, retrieving and/or caching the value if APC +# is available for use +# accepts an optioanal TTL value (defaults to 600 seconds) +function db_cache_value($dbq, $dbh, $key, $ttl=600) { + $status = false; + $value = get_cache_value($key, $status); + if (!$status) { + $result = db_query($dbq, $dbh); + $row = mysql_fetch_row($result); + $value = $row[0]; + set_cache_value($key, $value, $ttl); + } + return $value; +} + # set up the visitor's language # function set_lang() { diff --git a/web/lib/stats.inc b/web/lib/stats.inc index 756fa27..29ba0bb 100644 --- a/web/lib/stats.inc +++ b/web/lib/stats.inc @@ -2,40 +2,10 @@

include_once('aur.inc');

-# APC configuration variables -$apc_prefix = 'aur:'; -$apc_ttl = 600; - -# Check if APC extension is loaded -if (!defined('EXTENSION_LOADED_APC')) - define('EXTENSION_LOADED_APC', extension_loaded('apc')); - -# run a simple db query, retrieving and/or caching the value if APC -# is available for use -# -function db_cache_value($dbq, $dbh, $key) -{ - global $apc_ttl; - $bool = false; - if(EXTENSION_LOADED_APC) { - $ret = apc_fetch($key, $bool); - } - if(!$bool) { - $result = db_query($dbq, $dbh); - $row = mysql_fetch_row($result); - $ret = $row[0]; - if (EXTENSION_LOADED_APC) { - apc_store($key, $ret, $apc_ttl); - } - } - return $ret; -} - function updates_table($dbh) { - global $apc_prefix, $apc_ttl; - $key = $apc_prefix . 'recent_updates'; - if(!(EXTENSION_LOADED_APC && ($newest_packages = apc_fetch($key)))) { + $key = 'recent_updates'; + if(!($newest_packages = get_cache_value($key))) {

Any reason to use an additional variable for the key here?

$q = 'SELECT * FROM Packages ORDER BY ModifiedTS DESC LIMIT 10'; $result = db_query($q, $dbh);

@@ -43,26 +13,23 @@ function updates_table($dbh) while ($row = mysql_fetch_assoc($result)) { $newest_packages->append($row); } - if (EXTENSION_LOADED_APC) { - apc_store($key, $newest_packages, $apc_ttl); - } + set_cache_value($key, $newest_packages);

Looks like you introduced a whitespace mistake here.

} include('stats/updates_table.php'); }

function user_table($user, $dbh) { - global $apc_prefix; $escuser = mysql_real_escape_string($user); $base_q = "SELECT count(*) FROM Packages,Users WHERE Packages.MaintainerUID = Users.ID AND Users.Username='" . $escuser . "'";

$maintainer_unsupported_count = db_cache_value($base_q, $dbh, - $apc_prefix . 'user_unsupported_count:' . $escuser); + 'user_unsupported_count:' . $escuser);

$q = "SELECT count(*) FROM Packages,Users WHERE Packages.OutOfDateTS IS NOT NULL AND Packages.MaintainerUID = Users.ID AND Users.Username='" . $escuser . "'";

$flagged_outdated = db_cache_value($q, $dbh, - $apc_prefix . 'user_flagged_outdated:' . $escuser); + 'user_flagged_outdated:' . $escuser);

# If the user is a TU calculate the number of the packages $atype = account_from_sid($_COOKIE["AURSID"]); @@ -72,35 +39,33 @@ function user_table($user, $dbh)

function general_stats_table($dbh) { - global $apc_prefix; # AUR statistics $q = "SELECT count(*) FROM Packages"; - $unsupported_count = db_cache_value($q, $dbh, $apc_prefix . 'unsupported_count'); + $unsupported_count = db_cache_value($q, $dbh, 'unsupported_count');

$q = "SELECT count(*) FROM Packages WHERE MaintainerUID IS NULL"; - $orphan_count = db_cache_value($q, $dbh, $apc_prefix . 'orphan_count'); + $orphan_count = db_cache_value($q, $dbh, 'orphan_count');

$q = "SELECT count(*) FROM Users"; - $user_count = db_cache_value($q, $dbh, $apc_prefix . 'user_count'); + $user_count = db_cache_value($q, $dbh, 'user_count');

$q = "SELECT count(*) FROM Users,AccountTypes WHERE Users.AccountTypeID = AccountTypes.ID AND AccountTypes.AccountType = 'Trusted User'"; - $tu_count = db_cache_value($q, $dbh, $apc_prefix . 'tu_count'); + $tu_count = db_cache_value($q, $dbh, 'tu_count');

$targstamp = intval(strtotime("-7 days")); $yearstamp = intval(strtotime("-1 year"));

$q = "SELECT count(*) FROM Packages WHERE Packages.ModifiedTS >= $targstamp AND Packages.ModifiedTS = Packages.SubmittedTS"; - $add_count = db_cache_value($q, $dbh, $apc_prefix . 'add_count'); + $add_count = db_cache_value($q, $dbh, 'add_count');

$q = "SELECT count(*) FROM Packages WHERE Packages.ModifiedTS >= $targstamp AND Packages.ModifiedTS != Packages.SubmittedTS"; - $update_count = db_cache_value($q, $dbh, $apc_prefix . 'update_count'); + $update_count = db_cache_value($q, $dbh, 'update_count');

$q = "SELECT count(*) FROM Packages WHERE Packages.ModifiedTS >= $yearstamp AND Packages.ModifiedTS != Packages.SubmittedTS"; - $update_year_count = db_cache_value($q, $dbh, $apc_prefix . 'update_year_count'); + $update_year_count = db_cache_value($q, $dbh, 'update_year_count');

$q = "SELECT count(*) FROM Packages WHERE Packages.ModifiedTS = Packages.SubmittedTS"; - $never_update_count = db_cache_value($q, $dbh, $apc_prefix . 'never_update_count'); + $never_update_count = db_cache_value($q, $dbh, 'never_update_count');

include('stats/general_stats_table.php'); } -

On Sun, May 29, 2011 at 1:59 PM, elij <elij.mx@gmail.com> wrote:

On Sun, May 29, 2011 at 7:27 AM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:

...

On Sat, May 28, 2011 at 04:17:09PM -0700, elij wrote:

...

+    if(EXTENSION_LOADED_APC) { +        $ret = apc_fetch(APC_PREFIX.$key, $status); +        if ($status) { +            return $ret; +        } +    } +    return $status; +}

I'd prefer to change get_cache_value()'s signature to return the status indicator and pass the actual result by reference. That way, it could be used as follows:

---- if (get_cache_value('foo', $foo)) {  do_something $foo } ----

That just feels much more common and convenient. Any objections?

That seems a bit unconventional, considering the existing codebase and common php practices. While it might make for the occasional nice conditional-if test, I think it is more of a leaky abstraction than the existing method.

I made an attempt to match the api signature for apc_fetch and memcache::get, so that I had to change as little calling code as possible (while making the api somewhat expected).

The version as written behaves like the existing apc_fetch: http://www.php.net/manual/en/function.apc-fetch.php

And purposefully similarly to memcache::get (if no bool reference is passed in to get_cache_value): http://us.php.net/manual/en/memcache.get.php

Memcache::get returns false on failure, which can be problematic if a falsey value was stored in memcache, and you were trying to get it out (and test that retrieval succeeded). Passing a bool by reference fixes that case.

You _can_ use a convention like this:

   if(!($foo = get_cache_value('bar'))) {      // do stuff    }

As get_cache_value on failure *also* returns a falsey value, but this runs into the memcache api problem of what happens if you want to retrieve a falsey value. If you know that either you never store a falsey value for that key, or if your conditional test is appropriate assuming a falsey value stored (eg. if the condition is false due to failure or retrieval of a falseyness, the expected behavior is the same), then that convention works fine.

Passing a data container by reference (your suggestion) would also work fine, but I don't see that very often in practice. I am probably not very current on php conventions though, and I am using the php documentation as a reference for 'best practices for api signatures'. Which may be a fools errand to some extent. ;)

Do you make use of the 'pass data container by reference' convention regularly, or see it commonly used?

...

...

 function updates_table($dbh)  { -     global $apc_prefix, $apc_ttl; -     $key = $apc_prefix . 'recent_updates'; -     if(!(EXTENSION_LOADED_APC && ($newest_packages = apc_fetch($key)))) { +     $key = 'recent_updates'; +     if(!($newest_packages = get_cache_value($key))) {

Any reason to use an additional variable for the key here?

Nope. That was just how it was, and I missed changing it to inline. Note the diff.

Actually, I just looked at the file, and the key is used twice (slightly later in the same function). At the time I probably thought it was better to have a variable than multiple instances of the same string.

...

...

              $q = 'SELECT * FROM Packages ORDER BY ModifiedTS DESC LIMIT 10';               $result = db_query($q, $dbh);

@@ -43,26 +13,23 @@ function updates_table($dbh)               while ($row = mysql_fetch_assoc($result)) {                       $newest_packages->append($row);               } -             if (EXTENSION_LOADED_APC) { -                     apc_store($key, $newest_packages, $apc_ttl); -             } +        set_cache_value($key, $newest_packages);

Looks like you introduced a whitespace mistake here.

Ah yes. I have vim set to use tabs on php files, but the inc files do not end in php...  :/ Which as an aside is not a good practice. If for some reason there was a server configuration error, the inc files would be served up as plain text. It is best practice to have config files (such as config.inc) end in .php so they will be rendered in the off chance the file is exposed. The include files should probably be renamed.

I can fix this one line of whitespace and resubmit if desired.