When answering this question http://drupal.org/node/319803 I was reminded that this function doesn't implement any sort of cache. If userpoints_get_current_points is called in comment.tpl.php it could potentially reduce performance as there is a SELECT statement for each API call even if its the same user requested multiple times.
We can static cache this with something like...
function userpoints_get_current_points($uid = NULL, $tid = NULL) {
static $cache_current_points;
//existing $uid & $tid checking code
if ( is_numeric($cache_current_points[$uid][$tid]) ) {
return $cache_current_points[$uid][$tid];
}
// existing code to retrieve the userpoints
$cache_current_points[$uid][$tid] = $points;
return $points.
}
a cache_set, cache_get is overkill here as this isn't a persistent cache but rather preventing identical SQL queries on a single page request.
Thoughts/comments?
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | userpoints_319883.patch | 2.49 KB | catch |
| #6 | userpoints-319883.patch | 2.06 KB | catch |
| #3 | userpoints-319883.patch | 2.07 KB | catch |
Comments
Comment #1
kbahey commentedCache set and cache get would not save us much since it is still one query to get points for a given user.
Having it as a static array though can make it grow very large.
We had the same problem for path aliases in core a few releases ago, and once we exceeded a certain number, it became a real bottleneck, because building the array took so much time and memory too.
Perhaps limiting it to, say, 50 users would be reasonable. This would prevent the scalability issues.
Either way, we need to have benchmarks to confirm one way or the other. I can help with that once you have code to test.
Comment #2
berdiruserpoints_get_max_points() does have a static cache, so I guess it would make sense to have one for current_points() as well :)
Addressing the scalability issues (if there really are any, which I'm not sure about) could be as easy as conditionally checking if there are already 50 different entries in the cache and don't add it then. Not perfect, but should avoid issues when someone is calling it for hundreds of users for whatever reason.
Would also be interesting to see how often the function is actually called.
Oh, something that needs to be taken care of are the tests, which would be quite broken if this is implemented I think.
Dealing with cache_get()/set means that we also have to clear the cache every time the points are updated, so I agree that we should leave that out.
Comment #3
catchThe static cache is only going to be an array of uid/tid + the integer value of the points, unless you are loading thousands of users this shouldn't be a memory issue (especially compared to most things in D6).
Attaching a patch for this, not tested yet or anything.
Comment #5
berdirThe reset check here should be unecessary, because the check above returns early when $reset is TRUE.
semicolon is missing here :)
Powered by Dreditor.
Comment #6
catchThanks Berdir, updated patch.
Comment #8
catchFixed the test failures.
Comment #9
berdirCommited to 6.x-1.x. Noticed during porting to D7, that the cache reset was called *before* the points were saved. Changed that and also updated the apidocs a bit, see http://drupalcode.org/project/userpoints.git/commitdiff/899e983
Because of that, I could remove the existing, previously necessary reset static call in the tests, had to add another one however. I think the tests show why it makes more sense this way round: When using the API, the static cache managed correctly, when messing the the database tables directly, a manual static cache reset is necessary.