340160 - Speed up LogActivityEntry()

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Vlad Dascalu]

There are places in the code where we call several times LogActivityEntry().

The issue with that is that the SQL statement that inserts the data into the log table must be prepared each time LogActivityEntry() is called.

There are two optimisation possibles:

1) (suggested by LpSolit): make LogActivityEntry (or something similar) to accept an array with all the changes.
2) (suggested by me): make LogActivityEntry to prepare the INSERT statement in a static/global $sth, and reuse that if it is defined. This would speed things especially under mod_perl.

(this is a follow-up on bug 340139)

Comment 1

[Max Kanat-Alexander]

Okay. But is there any data that says that this actually causes any slowness in the code? That is, I'd want to see some profiling before we did any (possibly premature) optimization.

Comment 2

[Frédéric Buclin]

(In reply to comment #1)
> Okay. But is there any data that says that this actually causes any slowness in
> the code? That is, I'd want to see some profiling before we did any (possibly
> premature) optimization.
> 

This is not premature optimization. This makes perfect sense when deleting e.g. milestones where many bugs are reassigned at once with exactly the same arguments passed to LogActivityEntry().

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> This is not premature optimization.

  Okay. All real optimizations have profiling data to back them up. Show me a profile. If you need any help profiling perl, I've done it before and I can probably help you out a bit.

Comment 4

[Frédéric Buclin]

We are freezing the code for 3.0 in two weeks and we don't expect this bug to be fixed on time.

Comment 5

[Frédéric Buclin]

I did some profiling, and the perf win is very visible when editing several fields at once.

Comment 6

[Frédéric Buclin]

Attachment: patch, v1

Comment 7

[Frédéric Buclin]

Attachment: patch, v1.1

Also optimize get_field_id(), which is called by LogActivityEntry().

Comment 8

[:glob ✱]

Comment on attachment 808812 [details] [diff] [review]
patch, v1.1

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

::: Bugzilla/Field.pm
@@ +1334,5 @@
>  =cut
>  
>  sub get_field_id {
> +    return Bugzilla->fields({ by_name => 1 })->{$_[0]}->id
> +      or ThrowCodeError('invalid_field_name', {field => $_[0]});

when called with an invalid field name, this will die with 'Can't call method "id" on an undefined value' instead of throwing the invalid_field_name error.

Comment 9

[Frédéric Buclin]

Attachment: patch, v2

Oops yes. Thanks for catching that! :)

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 809064 [details] [diff] [review]
patch, v2

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

r=dkl

Comment 11

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Bug.pm
modified Bugzilla/Field.pm
Committed revision 8764.

Comment 12

[Frédéric Buclin]

Looks like using state $sth is only acceptable with mod_cgi, not mod_perl. Per my discussion in the #perl IRC channel:

(14:12:54) Botje: LpSolit: see if you can reproduce it first, maybe?
(14:13:12) Botje: also check if the database supports multiple queries at once with the same db handle, most don't.
(14:13:18) LpSolit: Botje: I cannot reproduce, but I use mod_cgi while the one who could reproduce says he uses mod_perl
(14:13:33) Botje: there's your problem.
(14:13:50) LpSolit: mod_perl?
(14:13:56) Botje: yes.
(14:14:21) Botje: if you're running mod_perl you're either a masochist or someone supporting legacy applications.
(14:14:26) buu: LpSolit: I certainly wouldn't use state, especially on a database handle, in a mod_perl application
(14:16:56) LpSolit: so there is no good way to define a prepared MySQL statement only once?
(14:17:13) LpSolit: I thought that was what state was used for
(14:17:14) Botje: LpSolit: state is a perfectly good solution. just not under mod_perl.
(14:17:54) pink_mist: LpSolit: best thing would be to simply not support mod_perl
(14:18:42) DrForr: Preparing a SQL statement is fine, it's just the "only once" bit that stands out.
(14:18:45) Botje: LpSolit: urgh. better run it under fastcgi. at least there lifetime is somewhat limited.
(14:20:03) Botje: you could give prepare_cached a try.
(14:20:21) Botje: that allows DBI to do some caching, but only per db handle.
(14:20:28) Botje: which is safer, i think.
(14:21:26) LpSolit: so you guys say that FastCGI is superior to mod_perl?
(14:21:42) Botje: LpSolit: pretty much. mod_perl is for modifying how the web server works, not for running applications.
(14:21:59) buu: mod_perl is ancient terrible dark magic.
(14:22:01) buu: We shun such things.
(14:27:51) Botje: LpSolit: build bugzilla on top of PSGI, that will let people run it on all kinds of backends.
(14:27:52) pink_mist: LpSolit: I'm personally more a fan of Mojolicious than any of the others; but mostly because that's the one I've used the most
(14:29:35) DrForr: LpSolit: Something as big as Bugzilla probably would gravitate toward Catalyst. Mojolicious might work well as well, but I don't have as much experience with that.


I have reverted the "state $sth" part for now, but kept changes in get_field_id(), which are fine:

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Bug.pm
Committed revision 8771.

Comment 13

[Frédéric Buclin]

Attachment: patch, v3

Now use prepare_cached().

Comment 14

[Frédéric Buclin]

I did some profiling and prepare_cached() shows a clear win compared to do(). And this way, there is no need to use |state $sth|.

Comment 15

[David Lawrence [:dkl]]

Comment on attachment 816562 [details] [diff] [review]
patch, v3

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

tested under mod_cgi and mod_perl and works as expected. r=dkl

Comment 16

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Bug.pm
Committed revision 8772.