Closed Bug 499585 Opened 16 years ago Closed 15 years ago

Cache all calls to Bugzilla->get_fields

Categories

(Bugzilla :: Bugzilla-General, enhancement)

3.2.3
enhancement
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: mkanat, Assigned: mkanat)

Details

(Keywords: perf)

Attachments

(1 file)

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
Attached patch v1Splinter Review
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?
(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?)
(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 on attachment 384306 [details] [diff] [review] v1 r- based on bbaetz's point about the separator.
Attachment #384306 - Flags: review?(LpSolit) → review-
We no longer accept new features for Bugzilla 3.6. Retargetting to 3.8.
Target Milestone: Bugzilla 3.6 → Bugzilla 3.8
Okay, this is too complex. Instead, I've decided to implement a new, simpler accessor called Bugzilla->fields in bug 576670.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → WONTFIX
Target Milestone: Bugzilla 4.0 → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: