Posted by Jeremy on September 30, 2009 at 6:27pm
| Project: | User Points |
| Version: | 6.x-1.x-dev |
| Component: | Code: userpoints API |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
While reviewing the performance of a client website, I noticed that the same identical queries from userpoints_get_max_points() are run multiple times on same pages, often >25 times. The attached patch fixes this.
On the page where I tested this, it resulted in ~100 fewer database queries to build the page.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| userpoints.module.patch | 1.04 KB | Ignored: Check issue status. | None | None |
Comments
#1
Fixing title.
#2
I like the idea, but I think your logic has a problem.
The function can be called for a different $uid each time. So caching has to take that into account.
Right now it will return the value for the first $uid that is cached regardless of which $uid is passed in.
Also, I think the code could be simpler. For example return early if found in the cache.
As well, roll it against what is in DRUPAL-6--1 cvs tag.
#3
> The function can be called for a different $uid each time. So caching
> has to take that into account.
Whoops! Indeed, an updated patch is attached.
> Also, I think the code could be simpler. For example return early if
> found in the cache.
I don't see any way to get a gain from this. The logic does the following:
1. if uid is not set, set it
2. if tid is not set, set it
3. if cached value is not set, set it
4. return cached value
You could add a 2.5 that returns if the cache is set, but that means 1) you'd add an if to save an if which saves nothing, and 2) you'd have to return twice in this function.
> As well, roll it against what is in DRUPAL-6--1 cvs tag.
Okay, done.
#4
Early return seems to be the norm in Drupaland, although like you, I used to shy away from it.
Anyway, it is now commited, with lots of comments for clarity.
#5
Automatically closed -- issue fixed for 2 weeks with no activity.