Cache all calls to Bugzilla->get_fields

RESOLVED WONTFIX

Status

()

--
enhancement
RESOLVED WONTFIX
10 years ago
8 years ago

People

(Reporter: mkanat, Assigned: mkanat)

Tracking

({perf})

Details

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
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
(Assignee)

Comment 1

10 years ago
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.
Assignee: general → mkanat
Status: NEW → ASSIGNED
Attachment #384306 - Flags: review?(LpSolit)
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?
(Assignee)

Comment 3

10 years ago
(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.
(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?)
(Assignee)

Comment 5

10 years ago
(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.
(Assignee)

Comment 6

9 years ago
Comment on attachment 384306 [details] [diff] [review]
v1

r- based on bbaetz's point about the separator.
Attachment #384306 - Flags: review?(LpSolit) → review-

Comment 7

9 years ago
We no longer accept new features for Bugzilla 3.6. Retargetting to 3.8.
Target Milestone: Bugzilla 3.6 → Bugzilla 3.8
(Assignee)

Comment 8

8 years ago
Okay, this is too complex. Instead, I've decided to implement a new, simpler accessor called Bugzilla->fields in bug 576670.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX
(Assignee)

Updated

8 years ago
Target Milestone: Bugzilla 4.0 → ---
You need to log in before you can comment on or make changes to this bug.