499585 - Cache all calls to Bugzilla->get_fields

Status: RESOLVED WONTFIX

Description

[Max Kanat-Alexander]

Right now lots of different places call get_fields, and they do it with lots of different parameters. We can cache all calls by:

1) Sorting the keys of the parameter hash
2) Joining the keys and values into one string
3) Using that string as a hash key for a cache

Comment 1

[Max Kanat-Alexander]

Created attachment 384306 [details] [diff] [review]
v1

Here we go. This seems to work just fine.

Note that you don't need to worry about us calling update() on a cached field and then having a stale cache somewhere--I checked the codebase and we never call update() on a Field object that came from get_fields.

Comment 2

[Bradley Baetz (:bbaetz)]

Comment on attachment 384306 [details] [diff] [review]
v1

What is the perf win from this? ie is this actually worth it?

>+    # $criteria_string is our hash key for caching. If there are no criteria,
>+    # it's an empty string.
>+    my $criteria_string = '';
>+    if ($criteria) {
>+        # We sort the keys and join them in order so that the ordering is
>+        # always consistent.
>+        my @criteria_keys = sort keys %$criteria;
>+        foreach my $key (@criteria_keys) {
>+            $criteria_string .= "$key" . $criteria->{$key};

I think you need to have some separating characters, else this can't tell the difference between |a=>'b',c=>'de'| and |a=>'bc',d=>'e'|.

What if the criteria is |undef|?

Are we better off doing this in Bugzilla::Field::match?

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> (From update of attachment 384306 [details] [diff] [review])
> What is the perf win from this? ie is this actually worth it?

  Well, I do know that the most common query on bmo is reading the fields table.

> I think you need to have some separating characters, else this can't tell the
> difference between |a=>'b',c=>'de'| and |a=>'bc',d=>'e'|.

  Oh, good point! :-)

> What if the criteria is |undef|?

  It handles that case already--it uses an empty string.

> Are we better off doing this in Bugzilla::Field::match?

  No, I think it's more sensible to do it here in the Bugzilla object, since that's where we're caching it.

Comment 4

[Bradley Baetz (:bbaetz)]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 384306 [details] [diff] [review] [details])
> > What is the perf win from this? ie is this actually worth it?
> 
>   Well, I do know that the most common query on bmo is reading the fields
> table.

But the same query, or different ones? active_custom_fields is common (and there may be a few templates that should be calling that instead of ::match)
 
> > What if the criteria is |undef|?
> 
>   It handles that case already--it uses an empty string.

But it can't separate the two cases (plus won't it give an |undef| error in the logs?)

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> But the same query, or different ones? active_custom_fields is common (and
> there may be a few templates that should be calling that instead of ::match)

  Well, we know that at least two queries (active_custom_fields and bug_fields) were causing a perf problem, and honestly I'd just like to be able to use this function everywhere without every worrying about performance.

> But it can't separate the two cases (plus won't it give an |undef| error in the
> logs?)

  I think you need to read the code again.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 384306 [details] [diff] [review]
v1

r- based on bbaetz's point about the separator.

Comment 7

[Frédéric Buclin]

We no longer accept new features for Bugzilla 3.6. Retargetting to 3.8.

Comment 8

[Max Kanat-Alexander]

Okay, this is too complex. Instead, I've decided to implement a new, simpler accessor called Bugzilla->fields in bug 576670.