560695 - clicking (more) widget in headers with lots of recipients too slow

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

This is a known regression from bug 550487.  Current working theory (thanks to DTrace) is that we're reflowing after each address we add.

Comment 1

[Ben Bucksch (:BenB)]

IIRC I saw evidence of this is related to the .collapsed state (see the patch there).

And the collapsed in turn is we think due to unused headers lines (CC/BCC/Reply-To) still being collapsed.
First step would be to remove that "uncollapse all parents" hack and fix it by preventing the node from being collapsed, e.g. by uncollapsing the needed header line earlier.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

This would be consistent with my current working theory, mentioned above: when things are collapsed, layout probably doesn't feel the need to reflow as much.

Comment 3

[Ben Bucksch (:BenB)]

IIRC, the evidence I saw pointed the other direction: it's slow in collapsed state and faster when uncollapsed. But I may have been confused.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: fix, v1

Some massaging of the code coaxed dtrace into revealing that reading .clientWidth was the thing that was taking literally 95% of the time, presumably because each read was causing Gecko to flush its state and reflow.

As it turns out, there's no need to do that in the case where (X more) has been clicked and we're displaying all the headers, because we no longer need to detect overflow in order to place the (X more) node.

So now we don't.  :-)

Given that there's nothing functionally different here, there doesn't seem to be much benefit in writing a test.  That said, I'll add an in-testsuite? flag, because once we actually have a performance testing framework, this would be a good candidate for that.

Comment 5

[Ben Bucksch (:BenB)]

Great!
r=BenB

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 444222 [details] [diff] [review]
fix, v1

>+++ b/mail/base/content/mailWidgets.xml
>@@ -405,23 +405,27 @@
>-              // Calculate width and lines

We seem to have lost this comment.  Was that on purpose?

Other than that, I like it.  It's understandable, and it makes things faster for me.

Later,
Blake.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: fix, v2 (checked in on 1.9.3 trunk and 1.9.2)

Added back the comment as requested by Blake and rebased against the Jim's patch from bug 563612, as I'll be landing them together.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Landed:

http://hg.mozilla.org/comm-central/rev/84dd77b453af
http://hg.mozilla.org/releases/comm-1.9.2/rev/000ce50ec4f3

Comment 9

[Joe Sabash [:JoeS1]]

Feedback:
Excellent Perf gain <1sec on "more".
Minor nit, if View>>Headers>>All only 1 cc per line is display.
(but maybe that's been there all along, not sure)

Comment 10

[Joe Sabash [:JoeS1]]

Attachment: View headers all

Comment 11

[rsx11m]

Attachment: View headers all (a bit wider)

> (comment #9) Excellent Perf gain <1sec on "more".

Agreed, this is much of an improvement!

> Minor nit, if View>>Headers>>All only 1 cc per line is display.
> (but maybe that's been there all along, not sure)

Not really, your window is just narrow enough so that only a single address fits into one line, then it is wrapped. If you widen it the second addresses should become visible. There is a boundary between the box showing the addresses and the ones with the buttons and menus, it will wrap as soon as
you cross it (red line - the message is old, pictured with today's build).

Comment 12

[Joe Sabash [:JoeS1]]

Attachment: Same window size and res>>normal

This is not a showstopper, by any means. See attachment.

Comment 13

[rsx11m]

My guess is that the alignment of the boxes shifts between Normal and All headers. Your example "Content-Transfer-Encoding:" would certainly move the boundaries of the left-most header-label box to the right, thus reducing the space left for the headers themselves, causing this borderline appearance.

Comment 14

[Mark Banner (:standard8)]

Dan: Is this fixed now? Should the comments after the landing be in follow-up bugs?

Comment 15

[rsx11m]

The comments on the alignment of the boxes are unrelated to this bug, this is
a different issue and should be handled separately if it's a problem.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Mark: good catch; I should have closed this earlier.  Agreed that the box alignment issue wants to be spun off to a separate bug.