877078 - shift bugmail generation to the jobqueue

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement)

Description

[:glob ✱]

generating the bugmail content for insertion into the jobqueue slows down processing of bugs.

this is most problematic for changes which trigger many emails (on bmo we see an average of 30 emails per change, but sometimes over 170 emails will be generated from a single change).

it should be possible to serialise the state of the bug in process_bug.cgi and insert that into the jobqueue.  the workers can pick that up, generate the email content from that, post-process and deliver.

we need to serialise the current state of the bug as there's a risk that the bug will be changed between the time the job is inserted into the queue and when it's picked up for delivery.  however it isn't practical to serialise everything to do with a bug, so we should just serialise fields which we put into bugmail.

it's possible that other changes will happen between queue pushing and delivery (such as users changing their real name, referenced bugs changing), however i think that's a cost we can take if it means quicker bug updates.

Comment 1

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #0)
> it should be possible to serialise the state of the bug in process_bug.cgi
> and insert that into the jobqueue.

I think we should rather create a sub-process and detach it from the parent so that the page can be rendered immediately and let emails being sent in the background independently of the page rendering.

See http://modperlbook.org/html/10-2-1-Forking-a-New-Process.html and the following few pages on how to do that. This way, installations not using jobqueue.pl can still benefit from this improvement.

A full example is given at http://modperlbook.org/html/10-2-5-A-Complete-Fork-Example.html

Comment 2

[:glob ✱]

(In reply to Frédéric Buclin from comment #1)
> I think we should rather create a sub-process and detach it from the parent
> so that the page can be rendered immediately and let emails being sent in
> the background independently of the page rendering.
>
> See http://modperlbook.org/html/10-2-1-Forking-a-New-Process.html and the
> following few pages on how to do that.

i have tried forking for our arecibo/sentry integration, and found it to cause lots of problems (even when following the modperl cookbook).  dbi handles need special attention (http://elenst.ru/pensieve/it-bits/perl-fork-dbi-and-inactivedestroy/), however the issue we hit were zombie processes were generated.

for sentry, i ended up serialising the data to a file calling system('command&') to process.

> This way, installations not using jobqueue.pl can still benefit from this improvement.

if a site isn't already running jobqueue, they probably aren't generating a volume of email where this becomes an issue.

forking is also a poor choice on windows.

Comment 3

[:glob ✱]

that said, i'll play with mod_perl+dbi+fork to see if i can get it going.
it would be a much simpler fix for this!

Comment 4

[:glob ✱]

after a day of investigations, the long story short on forking with mod_perl2 is: don't.

the document pointed to in comment 1 is for mod_perl1 (cleanup_for_exec() is a give away - that no longer exists).

i can get the forking and dbi detaching working, however i then have issues with apache restarting, which is an indication that the child process isn't fully detached from its parent.

http://stackoverflow.com/q/471681/953 is almost exactly what i'm doing, but everything i've read about forking with mod_perl2 says "don't do it, instead use a PerlCleanupHandler or spawn a sub-process with Apache2::SubProcess'.

i'm now experimenting with the PerlCleanupHandler, but it's looking like it would be easier/cleaner to serialise and use the jobqeueue at this stage.

Comment 5

[:glob ✱]

Attachment: patch v1

this patch does two things..

first it extends bugzilla::object to allow objects to be flattened to a hashref. this works by storing the keys of the hash that was used to initially bless the object, and serialising only those keys to a hashref. there's a new constructor (new_from_hash) which accepts that hashref and blesses it back into a normal object.

second it changes bugmail so the template generation is performed by the jobqueue daemon. the objects in the $vars hashref are flattened, and $vars is pushed onto the ts queue.

timing data:

the change i performed timing testing on takes ~1682ms to update without sending any bugmail (51 recipients, all with bugmail disabled). i made changes to the dependency list, so 2 bugs were being updated.

with bugmail enabled and without this patch the change took ~2443ms. with this patch, ~2059ms.

in other words email generation dropped from 761ms to 377ms.

Comment 6

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #5)
> with bugmail enabled and without this patch the change took ~2443ms. with
> this patch, ~2059ms.
> in other words email generation dropped from 761ms to 377ms.

How many emails have been sent? And what are the numbers with many bugmails being sent at the same time?

Comment 7

[:glob ✱]

(In reply to Frédéric Buclin from comment #6)
> How many emails have been sent? 

all testing was done with 51 recipients.
bugmail was disabled only to gather the baseline ~1682ms time.
all other timings were with bugmail enabled.

> And what are the numbers with many bugmails being sent at the same time?

not sure what you're asking here.

Comment 8

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #7)
> not sure what you're asking here.

51 is reasonable. What happens with e.g. 200 recipients?

Comment 9

[:glob ✱]

i added 200 CCs to a bug, then took average timings for adding a comment to the bug.

with bugmail disabled      :  850ms
with bugmail, without patch: 4014ms
with bugmail, with patch   : 2496ms

email generation time dropped from 3164ms to 1646ms.

Comment 10

[Simon Green]

Comment on attachment 759642 [details] [diff] [review]
patch v1

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

Two minor things to change. Other than that the code looks good, but I haven't tested it yet.

::: Bugzilla/Job/BugMail.pm
@@ +29,5 @@
> +}
> +
> +1;
> +
> +=head1 B<Methods in need of POD>

No, you need to write POD for new methods.

::: Bugzilla/Object.pm
@@ +152,5 @@
> +sub _serialisation_keys {
> +    my ($class, $object) = @_;
> +    my $cache = Bugzilla->request_cache->{serialisation_keys} ||= {};
> +    $cache->{$class} = [ keys %$object ] if $object;
> +    return @{ $cache->{$class} } if defined wantarray;

15:04 <@glob> i don't know why :) should probably just always return the array

Comment 11

[:glob ✱]

Attachment: 877078_2.patch

> ::: Bugzilla/Job/BugMail.pm
> > +=head1 B<Methods in need of POD>
> No, you need to write POD for new methods.

as they are subclasses of another module which already has all the relevant documentation, they don't need docs.  i've added them to the correct whitelist in the pod coverage test.

Comment 12

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/BugMail.pm
modified Bugzilla/JobQueue.pm
modified Bugzilla/Object.pm
added Bugzilla/Job/BugMail.pm
modified Bugzilla/Job/Mailer.pm
modified t/011pod.t
Committed revision 8678.

Comment 13

[Frédéric Buclin]

Added to relnotes for 5.0rc1.