945535 - When loading bugs with large number of attachments, $bug->attachments reloads all flags for each attachment even if preloaded

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[David Lawrence [:dkl]]

Two places in the Bugzilla/Attachments.pm code that are contradictory of each other and causes extra db hits to occur with large number of attachments.

Bugzilla::Attachments::get_attachments_by_bug:

    [...]
    # To avoid $attachment->flags to run SQL queries itself for each
    # attachment listed here, we collect all the data at once and
    # populate $attachment->{flags} ourselves.
    if ($vars->{preload}) {
        $_->{flags} = [] foreach @$attachments;
        my %att = map { $_->id => $_ } @$attachments;

        my $flags = Bugzilla::Flag->match({ bug_id      => $bug_id,
                                            target_type => 'attachment' });

        # Exclude flags for private attachments you cannot see.
        @$flags = grep {exists $att{$_->attach_id}} @$flags;

        push(@{$att{$_->attach_id}->{flags}}, $_) foreach @$flags;
    [...]

But in Bugzilla::Attachment::flags:

 sub flags {
    # Don't cache it as it must be in sync with ->flag_types.
    return $_[0]->{flags} = [map { @{$_->{flags}} } @{$_[0]->flag_types}];
 }

So I propose updating $attachment->flags to only load if the flags list is not already preloaded.

Patch coming.

dkl

Comment 2

[David Lawrence [:dkl]]

Attachment: 945535_1.patch

Comment 3

[Frédéric Buclin]

Comment on attachment 8342084 [details] [diff] [review]
945535_1.patch

I want to review it too as I wrote that code.

Comment 4

[:glob ✱]

Comment on attachment 8342084 [details] [diff] [review]
945535_1.patch

r=glob

Comment 5

[Frédéric Buclin]

Comment on attachment 8342084 [details] [diff] [review]
945535_1.patch

>+    # To avoid $attachment->flags and $attachment->flag_types to run SQL queries
>+    # itself

s/itself/themselves/


>+        foreach my $attachment (@$attachments) {
>+            $attachment->{flag_types} = [];
>+            foreach my $type (@$flag_types) {
>+                my $new_type = dclone($type);

As dclone() is a bit slow (called #attachments x #flag_types), you should clone the whole arrayref at once, i.e. write

        foreach my $attachment (@$attachments) {
            $attachment->{flag_types} = [];
            my $new_types = dclone($flag_types);
            foreach my $new_type (@$new_types) {

In my testing, this makes dclone() 30% faster (it's faster to loop in C than in Perl).


>+                $new_type->{flags} = [ grep($_->type_id == $new_type->id 

There is no need to check the type ID. Flags are already grouped by type ID in Bugzilla::Flag->_flag_types.


>+        $attachments = [sort {$a->id <=> $b->id} @$attachments];

$attachments is already sorted.


Your patch works fine, but the benefit is small in a medium installation. Still good to have.

Comment 6

[David Lawrence [:dkl]]

Attachment: 945535_2.patch

Comment 7

[Frédéric Buclin]

Comment on attachment 8355252 [details] [diff] [review]
945535_2.patch

Looks good. r=LpSolit

Comment 8

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk
modified Bugzilla/Attachment.pm
Committed revision 8850.