Closed
Bug 499585
Opened 16 years ago
Closed 15 years ago
Cache all calls to Bugzilla->get_fields
Categories
(Bugzilla :: Bugzilla-General, enhancement)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: mkanat, Assigned: mkanat)
Details
(Keywords: perf)
Attachments
(1 file)
2.82 KB,
patch
|
mkanat
:
review-
|
Details | Diff | Splinter Review |
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•16 years ago
|
||
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•16 years ago
|
||
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•16 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.
Comment 4•16 years ago
|
||
(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•16 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•16 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•16 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•15 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
Closed: 15 years ago
Resolution: --- → WONTFIX
Assignee | ||
Updated•15 years ago
|
Target Milestone: Bugzilla 4.0 → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•