Closed Bug 560695 Opened 10 years ago Closed 9 years ago

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

Categories

(Thunderbird :: Message Reader UI, defect)

x86
All
defect
Not set

Tracking

(blocking-thunderbird3.1 rc1+, thunderbird3.1 rc1-fixed)

RESOLVED FIXED
Tracking Status
blocking-thunderbird3.1 --- rc1+
thunderbird3.1 --- rc1-fixed

People

(Reporter: dmose, Assigned: dmose)

References

Details

(Keywords: perf, regression)

Attachments

(4 files, 1 obsolete file)

This is a known regression from bug 550487.  Current working theory (thanks to DTrace) is that we're reflowing after each address we add.
Flags: wanted-thunderbird+
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.
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.
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.
Assignee: nobody → dmose
Whiteboard: [ETA for review-ready patch: Friday, May 7]
Whiteboard: [ETA for review-ready patch: Friday, May 7] → [ETA for review-ready patch: Tuesday, May 11]
Attached patch fix, v1 (obsolete) — Splinter Review
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.
Attachment #444222 - Flags: review?(bwinton)
Whiteboard: [ETA for review-ready patch: Tuesday, May 11] → [has patch; needs review bwinton, landing]
Flags: in-testsuite?
Great!
r=BenB
Attachment #444222 - Flags: review+
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.
Attachment #444222 - Flags: review?(bwinton) → review+
Whiteboard: [has patch; needs review bwinton, landing] → [has reviewed patch, needs comment tweak & landing dmose]
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.
Attachment #444222 - Attachment is obsolete: true
Attachment #445483 - Flags: review+
Attachment #445483 - Attachment description: fix, v2 → fix, v2 (checked in on 1.9.3 trunk and 1.9.2)
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)
Attached image View headers all
> (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).
This is not a showstopper, by any means. See attachment.
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.
Dan: Is this fixed now? Should the comments after the landing be in follow-up bugs?
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.
Mark: good catch; I should have closed this earlier.  Agreed that the box alignment issue wants to be spun off to a separate bug.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.