Closed Bug 425315 Opened 12 years ago Closed 12 years ago

Implement full-time cache with instant invalidation

Categories

(addons.mozilla.org Graveyard :: Administration, defect, critical)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: morgamic, Assigned: wenzel)

References

Details

Attachments

(2 files, 2 obsolete files)

Memcache is cached for X seconds but it should cache indefinitely (no expire time) with instant invalidation upon any update to related records.

Currently updates aren't instant, taking CACHE_PAGES_FOR seconds to get propagated.
If this is what our next step is in the caching system, this needs to be one of our highest priorities.  I don't know why there aren't already a dozen dupe bugs when we've got a process like:

1) edit something on a form, click submit 
2) form is returned without any of your changes
3) your changes don't appear on the live site for _hours_
Severity: major → critical
Only recently has the cache timeout been increased -- so maybe that's why it hasn't been duped as much as other issues.  I agree this should be a main focus.
From an email: "[...]from an extension author's perspective, this is the most frustrating bit of interacting with AMO."

If I get time I might take a whack at an object cache this weekend.
I looked at the object-level caching this weekend.  It wouldn't be terrible to add the caching, but knowing when to expire the objects would be pretty ugly.  Not something that could be solved quickly.

So, I had another idea that I wrote a patch for.  I'm not thrilled with parts of it but it's a start.  Stuff I don't like (this will make sense if you read the patch):

1) AppModel doesn't have access to the controller's $param var.  There is good stuff in there.  I kind of want to write a fakeParam() function so we can use the Model more effectively.

2) This isn't an automatic solution, other than the current pages (since cake forms submit to themselves, we get that for free).  Otherwise we have to manually tie page x with page y.  Probably not too many of those, but still.

3) Along the lines of #2, updating stuff like previews doesn't make for an easy way to expire the add-on page because they're not tied together by the URL.

Anyway, I'm looking for comments on whether the whole idea is worth pursuing and, if so, how to make it better.

(Edit: thought while I was typing this.  Could we use the session to store modified pageCacheKeys?  Then we could build them whenever (ie. in the controller) and expire them when we get the chance...)
Attachment #312857 - Flags: review?
I didn't request review from anyone specific because I'm still looking for general ideas and comments.
Another side-effect of not having instant invalidation is that when we sandbox an add-on it takes up to 3 hours for it to take effect.
Blocks: 424339
Target Milestone: 3.x (triaged) → 3.4
Assignee: nobody → clouserw
Attachment #312857 - Flags: review?(morgamic)
Attachment #312857 - Flags: review?(laura)
Attachment #312857 - Flags: review?
Duplicate of this bug: 419703
Blocks: 371262
Written for a blog post, but seems more appropriate here.  A summary of the patch:

A user loads an add-on detail page.  We know all the queries we just ran on this page and the hashes we used to store them in memcache.  I make a unique, reproducible key that represents this page in memcache and store a serialized array of all the query hashes we just created.  The reproducible key is something simple, like "special:addon:view:1865" as opposed to our query hashes which are primarly made up of MD5 sums.

When the add-on author updates their add-on, we can look in memcache for a key like "special:addon:view:$addon_id" and if it comes back with an array of hashes, which identifies the add-on detail page that needs flushing.

This isn't an automatic system (we have to manually tell the add-on detail page to store the special memcache entry), but it would give immediate results to the pages we did flush and it wouldn't be too hard to manually associate the majority of the popular pages.
Duplicate of this bug: 434936
Target Milestone: 3.4 → 3.5.1
Target Milestone: 3.5.1 → 4.0.1
Just landed the v0.1 of the library for expiring content on the NS: http://svn.mozilla.org/libs/ns-api/

Bug 442366 is tracking progress on that.
Blocks: 450442
Wil - when we do the flush does it work with multiple memcache nodes?  delete seems to be server-specific.
(In reply to comment #11)
> Wil - when we do the flush does it work with multiple memcache nodes?  delete
> seems to be server-specific.

We should extend Config->Cache->delete() then and have it loop through all the servers.
I find the proposed model quite complicated because it is quite page-centered (instead of add-on centered which is what AMO is). I think instead of implementing a complicated half-automatic solution like this, we should decide on an object model that more closely fits what we are doing on AMO and can thus be much simpler.

I am thinking about dropping the database retrieval logic into the models (where it belongs) and just calling things like getAddon(123), getPreview(456) from the controller, and in the getAddon() model method we could memcache this particular object and associate it with an add-on.

At write time, we'd call "flush everything that's related to add-on 123" and that would happen.

I have to ponder it a little more, especially as far as lists are concerned (if we don't want to execute 100 single add-on SELECT queries, we would need to cache the entire list and associate it with 100 add-ons instead?) but I'd like to get some feedback on the general idea.

Again, I am proposing abstracting from single SQL queries, and caching entire objects instead because it's easier to understand and thus simpler to realize.

Note that since that could be implemented in parallel, migration could happen one "action" at a time.
Attached patch Object-based memcaching (obsolete) — Splinter Review
Wil asked for somebody to come up with an alternative, so here we go:

This patch contains an object-based memcaching framework. Some ideas are borrowed from the work Wil did already, others are new.

This framework is not automatic, it needs to be "used" by the other models (proof of concept follows shortly), by providing object-based retrieval functions for the controllers, and dropping these objects into the memcache before returning.

The writeCacheObject() function takes one or more "invalidation lists" ids that allows the to-be-cached object to be associated with event ids. These ids can be marked "to be flushed" in other parts of the code (most notably, after saving changes to an add-on) and will be cleaned up then by the framework.
Attachment #337231 - Flags: review?(clouserw)
This shows how it's used in the model. No worries, the patch looks bigger than it is:

This is basically a reincarnation of the getListAddons() function with the difference, that it stores addons as individual objects, which allows us to use the same cache object in different parts of the page (more cache hits for the win!), and returns drastically less unwanted data, unless requested (that part is really bug 452456).

The important parts for the cache model are relatively small:
- We switch off the query caching, and if we find the object cached, return it immediately.
- otherwise we build the addon object (with associated data, according to the requested "associations")
- eventually we write the cache object and associate it with the list "addon:$id".
- in the aftersave callback, when this addon was edited, we mark this very ID for flushing, where it is going to be picked up by the garbage man and thrown out.
- the work in the addons controller is trivial: we just use the new model function instead.

Other events causing this add-on to be flushed could be uploading a new version, adding an author, or similar.
Attachment #337237 - Flags: review?(clouserw)
r+ for the idea.  I like it a lot better than my patch.  For anyone not reading the patches, essentially it's creating another layer which creates the objects between our current code and the queries.

Comments on the code:

Please fill in the missing phpdoc stuff (@return, @param, etc.) and fix the lines that are there.

Your code needs comment #11 fixed too.

I'm having a hard time seeing the value of expiration lists.  What do you envision storing in them?
(In reply to comment #16)
> r+ for the idea.  I like it a lot better than my patch.

Thanks!

> Please fill in the missing phpdoc stuff (@return, @param, etc.) and fix the
> lines that are there.

Will do!

> Your code needs comment #11 fixed too.

In fact, I was reading through php.net's memcache documentation and I don't see any indication why a delete() call should not delete the desired object from all memcache nodes. So I think we are good. Do you see anything stating the contrary?

> I'm having a hard time seeing the value of expiration lists.  What do you
> envision storing in them?

Every to-be-cached object is put on one or more expiration lists. If a write command is executed anywhere else on the page, that code is responsible for flushing the right list (or lists). So disabling an add-on would lead to flushing this addon's exp. list (i.e., all objects associated with this add-on, which may be add-on data, but also lists etc.). --Or do you see a way to simplify this, yet keep it powerful enough so I can, say, have the recommended list flushed when I disable a recommended add-on?
> > Your code needs comment #11 fixed too.
> 
> In fact, I was reading through php.net's memcache documentation and I don't see
> any indication why a delete() call should not delete the desired object from
> all memcache nodes. So I think we are good. Do you see anything stating the
> contrary?

Nope - not sure how it works.  I guess we could test it and find out.

> 
> > I'm having a hard time seeing the value of expiration lists.  What do you
> > envision storing in them?
> 
> Every to-be-cached object is put on one or more expiration lists. If a write
> command is executed anywhere else on the page, that code is responsible for
> flushing the right list (or lists). So disabling an add-on would lead to
> flushing this addon's exp. list (i.e., all objects associated with this add-on,
> which may be add-on data, but also lists etc.). --Or do you see a way to
> simplify this, yet keep it powerful enough so I can, say, have the recommended
> list flushed when I disable a recommended add-on?

I think the lists make this more complicated than it needs to be.  Why are we caching the recommended list as a unit?  If it's made up of 5 add-ons let's just retrieve the 5 cached add-ons each time.  Why should it be 1 cached object made up of 5 cached add-ons?
(In reply to comment #18)
> > I don't see
> > any indication why a delete() call should not delete the desired object from
> > all memcache nodes. So I think we are good. Do you see anything stating the
> > contrary?
> 
> Nope - not sure how it works.  I guess we could test it and find out.

We could do that, or we ask the memcache devs about it. PHP's interface just wraps their API, I assume.

> I think the lists make this more complicated than it needs to be.  Why are we
> caching the recommended list as a unit?  If it's made up of 5 add-ons let's
> just retrieve the 5 cached add-ons each time.  Why should it be 1 cached object
> made up of 5 cached add-ons?

I agree, if we can make it easier, we should, however, maybe I didn't make it clear enough: Due to the "associations" you can have like 5 representations of the same add-on cached: one that just consists of name and id, another one with all the details, a third one with just a few details, but version and file information... in addition, every page locale generates a different add-on object in the cache. All of these objects would need to be flushed when the add-on is edited.
> > I think the lists make this more complicated than it needs to be.  Why are we
> > caching the recommended list as a unit?  If it's made up of 5 add-ons let's
> > just retrieve the 5 cached add-ons each time.  Why should it be 1 cached object
> > made up of 5 cached add-ons?
> 
> I agree, if we can make it easier, we should, however, maybe I didn't make it
> clear enough: Due to the "associations" you can have like 5 representations of
> the same add-on cached: one that just consists of name and id, another one with
> all the details, a third one with just a few details, but version and file
> information... in addition, every page locale generates a different add-on
> object in the cache. All of these objects would need to be flushed when the
> add-on is edited.

I'd rather define an add-on object having consistent attributes even if some of them are empty and/or unused in some places.  That would simplify your code further also.
(In reply to comment #20)
> I'd rather define an add-on object having consistent attributes even if some of
> them are empty and/or unused in some places.  That would simplify your code
> further also.

All right -- even though I am uncomfortable to drag around, say, all versions of each add-ons on a page where none of them are needed.

Also, how do you handle the multiple languages problem? Do you store all of these in the same object as well?
Umm, yeah.  There's that.

We could have set keys that we always expire with an update like addon:$id:versions or addon:$id:$locale.  Then if we need the extra data we could just array_merge them with our actual addon:$id object.  This doesn't really seem like an improvement over arbitrary expire lists though. :-/
I updated the PHPdoc comments as requested.

I decided not to change the expiration lists stuff for now, as I think its "API" is quite easy to use (when storing an object you just tell it what list(s) to put it in) so it won't make the other code more complicated, I think.

Guess I need to write a few tests then before we can get this code in?
Assignee: clouserw → fwenzel
Attachment #337231 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #337931 - Flags: review?(clouserw)
Attachment #337231 - Flags: review?(clouserw)
> I decided not to change the expiration lists stuff for now, as I think its
> "API" is quite easy to use (when storing an object you just tell it what
> list(s) to put it in) so it won't make the other code more complicated, I
> think.
> 
I don't have a better way in mind. I think we should go ahead with what you've got.
Comment on attachment 337931 [details] [diff] [review]
Object-based memcaching, v2

Let's do some perf tests too
Attachment #337931 - Flags: review?(clouserw) → review+
(In reply to comment #25)
> Let's do some perf tests too

I agree! I should update the other patch so the object caching is used consistently for all add-ons shown on the front page (conveniently fixing bug 452456 at the same time).

Then, we could test an instance of the old vs. the new implementation. If you know where/how to do that, let me know.
(In reply to comment #26)
> (In reply to comment #25)
> > Let's do some perf tests too
> 
> I agree! I should update the other patch so the object caching is used
> consistently for all add-ons shown on the front page (conveniently fixing bug
> 452456 at the same time).
> 
> Then, we could test an instance of the old vs. the new implementation. If you
> know where/how to do that, let me know.

oremj has a test cluster he uses for benchmarks.  As far as I know he's done most of the AMO ones so far.
Webdev took over the test cluster a couple months ago.
(In reply to comment #17)
> > Your code needs comment #11 fixed too.

I read the C source of PHP's memcache extension and it seems, all cache keys are hashed and then deterministically sent to a specific server in the pool (which, if it's a cryptographic hash function, will lead to equal use of all servers in the pool), from which they are also deleted when delete() is called. Thus there's no reason for us to loop through servers when deleting.

Flush(), however, does not automatically seem to cover all servers, which is why Mike's implementation of bug 420921 makes sense (unless I missed something).
I wasn't sure and had not looked through the C code, but since it's an admin function manually connecting to each server in a loop was fine w/ me.

Thanks for checking on that.

BTW - I added a close statement just to make sure to close the connection:

Index: models/memcaching.php
===================================================================
--- models/memcaching.php	(revision 18160)
+++ models/memcaching.php	(working copy)
@@ -133,6 +133,7 @@
             if (!$m->connect($server,$params['port']) || !$m->flush()) {
                 return false;
             }
+            $m->close();
         }
         return true;
     }
Err, that was for bug 420921
Comment on attachment 337237 [details] [diff] [review]
Using object caching in the add-on model

I'll redo this patch with more consistent use of the function in bug 450442.
Attachment #337237 - Attachment is obsolete: true
Attachment #337237 - Flags: review?(clouserw)
This is checked in to r18173. Note, as it's the framework only, this patch doesn't do anything on its own. It has to be used in other places, such as the aforementioned bug 450442.

Leaving this open until I make a few tests.
Keywords: push-needed
I added a handful of tests to the memcaching model tests in r18191. Marking this fixed.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Keywords: push-needed
Comment on attachment 312857 [details] [diff] [review]
request for comments: just an idea

retroactive++
Attachment #312857 - Flags: review?(morgamic) → review+
Attachment #312857 - Flags: review?(laura)
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.