Closed Bug 237498 Opened 20 years ago Closed 10 years ago

Use memcached to improve performance

Categories

(Bugzilla :: Bugzilla-General, enhancement)

2.17.6
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 5.0

People

(Reporter: mkanat, Assigned: glob)

References

()

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

I think that memcached <http://www.danga.com/memcached/> could solve a lot of
Bugzilla's performance problems. From my understanding, after we work under
mod_perl, the next barrier is the database, and memcached is pretty good at
solving this problem. It solves it for LiveJournal.com, which has 20+ million
dynamic page views a day, in a basically perl-based system.

I think it would be nice to put a "usememcached" param into Bugzilla, and then
put hooks around in important places that would use memcached if the param was
turned on.

You can see the web site for more info about memcached.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.
Assignee: justdave → general
QA Contact: mattyt-bugzilla → default-qa
Note that before this can be reasonably done, we have to use Bugzilla::Object everywhere for updating and creating objects in Bugzilla.

Otherwise we wouldn't have any good way of doing cache invalidation. We'd have to add code to invalidate the cache after every single UPDATE and INSERT statement in Bugzilla, and we'd have to make sure that people keep doing it in the future, which isn't a workable solution.
we're hitting a point performance-wise where memcache is highly appealing to large sites, such as bmo.

> Otherwise we wouldn't have any good way of doing cache invalidation. We'd
> have to add code to invalidate the cache after every single UPDATE and
> INSERT statement in Bugzilla, and we'd have to make sure that people keep
> doing it in the future, which isn't a workable solution.

while we're in a significantly better position with regards to usage of bugzilla::object than we were 5 years ago, this is still and will continue to be a concern, especially with extensions code outside of the bugzilla core.
Assignee: general → glob
Blocks: 885464
Attached patch 237498_1.patch (obsolete) — Splinter Review
add preliminary memcached support.

currently only bz_schema is using the cache, however code exists in Bugzilla::Object for any subclasses to use the cache by setting USE_MEMCACHED to true.

an excerpt from the perldocs:

> The main driver for Memcached integration is to allow Bugzilla::Object
> based objects to be automatically cached in Memcache. This is enabled on a
> per-package basis by setting the "USE_MEMCACHED" constant to any true
> value.
> 
> The current implementation is an opt-in (USE_MEMCACHED is false by
> default), however this will change to opt-out once further testing has
> been completed (USE_MEMCACHED will be true by default).
Attachment #8339137 - Flags: review?(dkl)
Comment on attachment 8339137 [details] [diff] [review]
237498_1.patch

>=== modified file 'Bugzilla/DB.pm'

>+    $self->{private_real_schema} =
>+        $self->_bz_schema->deserialize_abstract($bz_schema->[0], $bz_schema->[1]);

From what I remember, deserializing the DB schema is the slow part. Why not cache it at the same time as you cache bz_schema? If bz_schema remains unchanged, so will the deserializated schema.



>=== modified file 'Bugzilla/Install/Requirements.pm'

>+        package => 'Cache-Memcached',
>+        module  => 'Cache::Memcached',
>+        version => '0',
>+        feature => ['memcached'],

I think you should require 1.27 at minimum (released on 2009-09-22), to handle UTF8 keys correctly. Or even 1.28 (released one month later) to support IPv6.



>=== modified file 'Bugzilla/Object.pm'

>+# Memcached. This will be flipped to true by default once the majority of
>+# Bugzilla Object have been tested with Memcached.
>+use constant USE_MEMCACHED => 1;

Isn't this supposed to be 0 for now?
Keywords: perf
Target Milestone: --- → Bugzilla 5.0
(In reply to Frédéric Buclin from comment #5)
> From what I remember, deserializing the DB schema is the slow part. Why not
> cache it at the same time as you cache bz_schema? If bz_schema remains
> unchanged, so will the deserializated schema.

avoiding hitting the database is the primary goal for the first revision.  happy to expand the scope in other bugs once we're happy with the base implementation.

> I think you should require 1.27 at minimum (released on 2009-09-22), to
> handle UTF8 keys correctly. Or even 1.28 (released one month later) to
> support IPv6.

will do :)

> >+use constant USE_MEMCACHED => 1;
> 
> Isn't this supposed to be 0 for now?

oops, yes.  i'll address that in the next revision.
Finding some strangeness in my testing that I want to see if you can reproduce on your end. I enabled memcache for Bugzilla::User objects only at first. I also enabled debug => 1 for the Cache::Memcached instance in Bugzilla::Memcached. For Bug users such assigned_to, qa_contact, etc. caching works properly in that I can see that it set()'s and then all future access is get() only. But for all Bugzilla::Flag::setter() access it is always a cache miss. I see in the debug output that it does the set() initially for the setter user, and then each get() fails and it does set() again and again even though the params are always the same. One thing I did notice when do Dumper on the params before it does the internal _set() is that when Bugzilla::Flag::setter is called, the 'id' param is a string value such as '768' where when other calls hit the cache, the 'id' value is an integer instead such as 768. It could be that the get() call is casting the id value differently when testing for a cache hit than what set() actually stores in the Memcached data? The other similar calls to Bugzilla::User->new are matching properly. Thoughts?

Some issue that may be related but is quite old and probably fixed:
https://github.com/facebook/hhvm/issues/372

dkl
Flags: needinfo?(glob)
(In reply to David Lawrence [:dkl] from comment #7)
> Finding some strangeness in my testing that I want to see if you can
> reproduce on your end. I enabled memcache for Bugzilla::User objects only at
> first. I also enabled debug => 1 for the Cache::Memcached instance in
> Bugzilla::Memcached. For Bug users such assigned_to, qa_contact, etc.
> caching works properly in that I can see that it set()'s and then all future
> access is get() only. But for all Bugzilla::Flag::setter() access it is
> always a cache miss.

i can't reproduce this issue.

31:23 MemCache: got prefix = 1387168268
31:23 MemCache: got 1387168268:bz_schema = ARRAY(0x445be38)
31:23 MemCache: got 1387168268:profiles.id.2 = HASH(0x4f63c30)
31:23 MemCache: got 1387168268:profiles.id.15188 = HASH(0x5083128)
31:23 MemCache: got 1387168268:profiles.id.56263 = HASH(0x5135f68)

user#2 is the assignee
user#15188 is the reporter
user#56263 is the setter of two flags (one bug, and one attachment).

running |memcached -vv| shows the same story.

> Some issue that may be related but is quite old and probably fixed:
> https://github.com/facebook/hhvm/issues/372

the memcache keys are always strings, never just an integer, so that issue isn't relevant.
Flags: needinfo?(glob)
Ok. Chalk another up to version difference. Centos6 comes with memcached version 1.4.4. Once I installed another memcached docker image that had version 1.4.13 and connected to it instead, the issue went away. I assume you are
also using a newer memcached since you develop on Fedora instead of Centos6. We need to see about getting newer memcached rpms built for RHEL6 when we roll this out to production. Continuing on with testing, shouldn't be long now.

dkl
Comment on attachment 8339137 [details] [diff] [review]
237498_1.patch

Review of attachment 8339137 [details] [diff] [review]:
-----------------------------------------------------------------

Little bit rot from bug 815026:

patching file Bugzilla/Object.pm
Hunk #3 succeeded at 499 with fuzz 2 (offset 12 lines).
Hunk #4 FAILED at 506.
1 out of 4 hunks FAILED -- saving rejects to file Bugzilla/Object.pm.rej

Otherwise looks really good and works as expected. Quick r+ with an updated patch.

::: Bugzilla/Memcached.pm
@@ +118,5 @@
> +    }
> +
> +    else {
> +        ThrowCodeError('params_required', { function => "Bugzilla::Memcached::clear",
> +                                            params   => [ 'key', 'table', 'object' ] });

What is 'object' as I do not see that as a possible param?

@@ +208,5 @@
> + my $data = $memcached->get({ key => 'data_key' });
> + if (!defined $data) {
> +     # not in cache, generate the data and populate the cache for next time
> +     $data = some_long_process();
> +     $memcached->set({ key => 'data_key' });

$memcached->set({ key => 'data_key', value => $data });

::: Bugzilla/Object.pm
@@ +36,5 @@
>  
> +# When USE_MEMCACHED is true, the class is suitable for serialisation to
> +# Memcached. This will be flipped to true by default once the majority of
> +# Bugzilla Object have been tested with Memcached.
> +use constant USE_MEMCACHED => 1;

Default to 0 as per bug comments
Attachment #8339137 - Flags: review?(dkl) → review-
Attached patch 237498_2.patchSplinter Review
addresses review comments.

following our discussion on irc, i also rename "_cache_*" to "_object_cache_*" to avoid confusion.
Attachment #8339137 - Attachment is obsolete: true
Attachment #8349190 - Flags: review?(dkl)
Comment on attachment 8349190 [details] [diff] [review]
237498_2.patch

Review of attachment 8349190 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I made you rename the cache related functions in Object.pm :(

Server Error: Can't locate object method "cache_key" via package "Bugzilla::User" at Bugzilla/Object.pm line 215

Fix on commit, everything looks good after fixing the above. r=dkl
Attachment #8349190 - Flags: review?(dkl) → review+
Flags: approval?
Flags: approval? → approval+
Could someone (ideally glob) reword the bug summary as the patch implements no hooks at all, at least not using Bugzilla::Hook.
Status: NEW → ASSIGNED
Keywords: relnote
(In reply to Frédéric Buclin from comment #13)
> Could someone (ideally glob) reword the bug summary as the patch implements
> no hooks at all, at least not using Bugzilla::Hook.

It is a hook in that it is harmless if Memcached is not set up and running. But not a hook in the sense of how we think of hooks using Bugzilla::Hook. I will reword the summary so it is more apparent.

dkl
Summary: hooks for memcached → Framework for using memcached to improve performance
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla.pm
modified checksetup.pl
modified Bugzilla/Bug.pm
modified Bugzilla/DB.pm
added Bugzilla/Memcached.pm
modified Bugzilla/Object.pm
added Bugzilla/Config/Memcached.pm
modified Bugzilla/Install/Requirements.pm
added template/en/default/admin/params/memcached.html.tmpl
modified template/en/default/setup/strings.txt.pl
Committed revision 8834.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Summary: Framework for using memcached to improve performance → Use memcached to improve performance
Blocks: 955962
Added to relnotes for 5.0rc1.
Keywords: relnote
Blocks: 1155009
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: