Add a caching mechanism to Bugzilla::Object to avoid querying the database repeatedly for the same information

RESOLVED FIXED in Bugzilla 5.0

Status

()

Bugzilla
Bugzilla-General
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: glob, Assigned: glob)

Tracking

({perf})

unspecified
Bugzilla 5.0
Dependency tree / graph
Bug Flags:
approval +
approval4.4 -

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Bugzilla::Bug->new() is called multiple times when displaying a bug.
each call results in a round trip to the database to fetch the bug's row.
this is inefficient.

i picked a bug at random from my test database, and it resulted in 13 calls to Bugzilla::Bug->new($bug_id), where $bug_id is the is of the bug i was viewing.

the breakdown is:

1 from show_bug.cgi
1 for each attachment (via Attachment->flag_types)
1 for each get_bug_link (this bug happened to have a lot of self-referential comments)
2 for each bug_format_comment (splinter extension)


we should only load a bug once from the database, then reuse the same object.
(Assignee)

Comment 1

6 years ago
Created attachment 681024 [details] [diff] [review]
patch v1

tested with landfill's database, bug 9855, mod_cgi

without patch: average 1286ms
   with patch: average 808ms
Attachment #681024 - Flags: review?(dkl)
(Assignee)

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86 → All
Summary: Bugzilla::Bug->new() is called multiple times when displaying a bug → Bugzilla::Bug->new() queries the database multiple times for the same information when displaying a bug

Comment 2

6 years ago
Comment on attachment 681024 [details] [diff] [review]
patch v1

You cannot cache bug objects this way. On bug creation, $self->{error} is set based on user privs. Same for $self->{choices}, $self->{tags}, $self->{actual_time}, etc... which are all user-dependent. Once a bug object is cached, it can be shared, and you may access data from someone else if the user accessing the data changes (think bugmails). This would open the door for many new security bugs.
Attachment #681024 - Flags: review?(dkl) → review-

Updated

6 years ago
Severity: major → enhancement
(Assignee)

Comment 3

6 years ago
this is a bug, not an enhancement.
Severity: enhancement → major
(Assignee)

Comment 4

6 years ago
(In reply to Frédéric Buclin from comment #2)
> You cannot cache bug objects this way. On bug creation, $self->{error} is
> set based on user privs. Same for $self->{choices}, $self->{tags},
> $self->{actual_time}, etc... which are all user-dependent. Once a bug object
> is cached, it can be shared, and you may access data from someone else if
> the user accessing the data changes (think bugmails). This would open the
> door for many new security bugs.

as far as i can tell, when Bugzilla->new() is called, Bugzilla->user is always the current user.  $user.can_see_bug() is used to determine visibility, including during bugmail generation.  Bugzilla.set_user() isn't called that much at all.

regardless, i'll provide an updated patch which includes the user's id as part of the cache's key.
(Assignee)

Comment 5

6 years ago
Created attachment 681073 [details] [diff] [review]
patch v2
Attachment #681024 - Attachment is obsolete: true
Attachment #681073 - Flags: review?(dkl)
Keywords: perf
Comment on attachment 681073 [details] [diff] [review]
patch v2

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

r=dkl
Attachment #681073 - Flags: review?(dkl) → review+

Updated

6 years ago
Flags: approval?
Flags: approval4.4?
(Assignee)

Updated

6 years ago
See Also: → bug 811630
(Assignee)

Updated

6 years ago
See Also: → bug 811650

Comment 7

6 years ago
Comment on attachment 681073 [details] [diff] [review]
patch v2

Has this patch been tested? You can no longer edit bugs. All changes are ignored.
Attachment #681073 - Flags: review-

Comment 8

6 years ago
Also, this patch doesn't let us recreate a bug object using the same bug ID, which is a problem in process_bug.cgi:

        if ($action eq 'same_bug') {
            # $bug->update() does not update the internal structure of
            # the bug sufficiently to display the bug with the new values.
            # (That is, if we just passed in the old Bug object, we'd get
            # a lot of old values displayed.)
            $bug = new Bugzilla::Bug($bug->id);
            $vars->{'bug'} = $bug;
        }

We need to be able to invalidate the cache, else the code above won't work anymore.
Flags: approval?
Flags: approval4.4?
(Assignee)

Comment 9

6 years ago
(In reply to Frédéric Buclin from comment #7)
> Has this patch been tested? You can no longer edit bugs. All changes are
> ignored.

yes, i tested this bug :|

looks like the bug is displayed with the updated values after you update it, but the database isn't actually updated.

Comment 10

6 years ago
Same problem in attachment.cgi:

  # We cannot reuse the $bug object as delta_ts has eventually been updated
  # since the object was created.
  $vars->{'bugs'} = [new Bugzilla::Bug($bugid)];

And that's only the two places I have in mind. I didn't check all places where we (re)create a bug object. Your fix is prone to regressions.
Severity: major → normal
(Assignee)

Comment 11

6 years ago
Created attachment 683001 [details] [diff] [review]
patch v3

while i still believe the default behavour for bugzilla should be to avoid re-creating objects from the database if we already have the data, this patch follows lpsolit's suggested path.

this patch extends bugzilla::object which allows the caller to indicate they would be happy with a cached object.  eg.
  $self->{product_obj} ||=
    new Bugzilla::Product({ id => $self->{product_id}, cache => 1 });

it also updates all places where we'd benefit from a cached object while viewing a bug or attachment.
Attachment #681073 - Attachment is obsolete: true
Attachment #683001 - Flags: review?(dkl)
(In reply to Byron Jones ‹:glob› from comment #11)
> while i still believe the default behavour for bugzilla should be to avoid
> re-creating objects from the database if we already have the data

+1... we should cache by default and allow objects to do cache => 0 if they absolutely need to bypass the cache.
(In reply to Reed Loden [:reed] from comment #12)
> +1... we should cache by default and allow objects to do cache => 0 if they
> absolutely need to bypass the cache.

we would likely need to heavily audit the usage if we implement this way...  caching by default is asking for data leakage if one place is attempting to filter on what the user's allowed to see and another isn't.
(Assignee)

Comment 14

6 years ago
(In reply to Dave Miller from comment #13)
> we would likely need to heavily audit the usage if we implement this way... 
> caching by default is asking for data leakage if one place is attempting to
> filter on what the user's allowed to see and another isn't.

agreed, however bear in mind we're just caching the individual objects, and for bugs the cache key includes the user's id.  by and large the visibility of data is governed not by the objects themselves, rather by the consumers, so they shouldn't be impacted by caching.
Good to know we're being that smart with the cache objects :)
Comment on attachment 683001 [details] [diff] [review]
patch v3

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

I do not see where you are including the user id with the cache_key value which was discussed in previous comments as being needed.

::: Bugzilla/Object.pm
@@ +117,4 @@
>      return $object;
>  }
>  
> +# Provides a mechanism for objects to be cached in the request_cahce

request_cache
Attachment #683001 - Flags: review?(dkl) → review-
(Assignee)

Comment 17

6 years ago
Created attachment 683586 [details] [diff] [review]
patch v4

odd; i definitely remember writing that code :|
no matter, here an updated patch which includes the missing sub.
Attachment #683001 - Attachment is obsolete: true
Attachment #683586 - Flags: review?(dkl)
Comment on attachment 683586 [details] [diff] [review]
patch v4

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

Looks good and works as expected. r=dkl
Attachment #683586 - Flags: review?(dkl) → review+
(Assignee)

Updated

6 years ago
Flags: approval?
Flags: approval4.4?

Comment 19

6 years ago
It's too late for 4.4, and my comments in this bug show that this code is prone to regressions. Security is also something which needs to be investigated carefully as it may result in leaks at unexpected places. This means much more testing than what I want to do post-RC.
Status: NEW → ASSIGNED
Flags: approval?
Flags: approval4.4?
Flags: approval4.4-
Flags: approval+
Keywords: relnote
Target Milestone: --- → Bugzilla 5.0
(Assignee)

Comment 20

6 years ago
(In reply to Frédéric Buclin from comment #19)
> It's too late for 4.4, and my comments in this bug show that this code is
> prone to regressions.

this statement isn't accurate -- your prior comments relate only to earlier patch versions, which enable caching by default.

at your request, the latest patch does caching on demand, which has *significant* less risk with regards to regressions.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 811630
(Assignee)

Updated

6 years ago
Duplicate of this bug: 811650
(Assignee)

Comment 23

6 years ago
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/
modified attachment.cgi
modified show_bug.cgi
modified Bugzilla/Attachment.pm
modified Bugzilla/Bug.pm
modified Bugzilla/Comment.pm
modified Bugzilla/Object.pm
modified Bugzilla/Product.pm
modified Bugzilla/Template.pm
Committed revision 8483.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
See Also: bug 811630, bug 811650
Summary: Bugzilla::Bug->new() queries the database multiple times for the same information when displaying a bug → Add a caching mechanism to Bugzilla::Object to avoid querying the database repeatedly for the same information
(Assignee)

Updated

6 years ago
Blocks: 815026

Updated

5 years ago
Blocks: 825718

Updated

5 years ago
Blocks: 829273

Updated

5 years ago
Blocks: 816333

Comment 24

3 years ago
This was already added to relnotes for 5.0rc1. Forgot to report this here.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.