732144 - Make caching of message-header address nodes with maxAddressesBeforeMore work again

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[:aceman]

Is see this in current trunk:
Warning: reference to undefined property this.maxAddressesBeforeMore
Source File: chrome://messenger/content/mailWidgets.xml
Line: 820

The code:
// We want to keep around the first maxAddressesBeforeMore email
// address nodes as well as any intervening comma nodes.
var numItemsToPreserve = this.maxAddressesBeforeMore * 2 - 1;
var numItemsInNode = aParentNode.childNodes.length;

This variable is defined nowhere. What should it be?

Comment 1

[rsx11m]

That's a remainder of bug 499989 and was replaced with this.maxLinesBeforeMore later (which was actually done for 3.1.x already, thus it's interesting that apparently nobody has noticed it before). As far as I recall, numItemsToPreserve was calculated to avoid throwing away too many address nodes which have to be repopulated later with the next message, so it shouldn't cause more than a performance issue due to unnecessary removals of address nodes.

BTW: I don't see any messages in the error console with 10.0.2 on Windows 7, potentially something was changed in Core now causing this to be exposed.

Comment 2

[:aceman]

I think making a debug build exposes many warnings like this. See also bug 731889. So it is entirely possible the error is there since 3.1 but you will never see it if you only use the official releases. Bug 499989 seems to add the maxLinesBeforeMore variable. But when did it vanish and what should we do today?

Comment 3

[rsx11m]

> Bug 499989 seems to add the maxLinesBeforeMore variable.

Actually, that was bug 550487 replacing the address count by counting lines.
Explanation from http://hg.mozilla.org/comm-central/rev/86fbaadd2cf3:

>    2.148 -      <!-- The number of addresses buildViews will display before adding a
>    2.149 -           (more) indicator to the widget; the nodes associated with these
>    2.150 -           addresses are cached for the lifetime of the widget. -->
>    2.151 -      <field name="maxAddressesBeforeMore">1</field>
>    2.152 +      <!-- The number of lines of addresses we will display before adding a
>    2.153 +           (more) indicator to the widget. This can be increased using the
>    2.154 +           preference mailnews.headers.show_n_lines_before_more .
>    2.155 +           This will also influence how many address elements are cached
>    2.156 +           for the lifetime of the widget. -->
>    2.157 +      <field name="maxLinesBeforeMore">1</field>

Well, the good news should be that it doesn't seem to do any harm, but I agree that it should be cleaned up. The problem is that the current "clearChildNodes" code assumes that the number of addresses before the "more" node is a constant, which was true for 3.0 but no longer 3.1 and later.

So, what exactly is it doing now? If "maxAddressesBeforeMore" defaults to 0 that method would simply remove all address nodes and they have to be reallocated when a new header is created (e.g., when moving to another message). If the loop isn't executed at all due to an undefined value, all nodes would be retained and just hidden if the new header doesn't use up all address nodes with a short "To:" or "Cc:" list (that's line #653 in the current trunk code). It's not necessarily a memory leak but a large chunk of memory may be hanging around after a large message header.

Given that "numItemsToPreserve" is hard to calculate with a varying number of addresses per line, maybe reverting to the status pre bug 499989 to simply have a constant cache size is the way to go, but then again a value of "3" would be of rather marginal impact on the performance of populating the address nodes...

See http://hg.mozilla.org/comm-central/rev/c49f355aed4d for that change:
>     1.52 -      <!-- as a perf optimization we are going to keep a cache of email address nodes which we've
>     1.53 -           created around for the lifetime of the widget. mSizeOfAddressCache controls how many of these
>     1.54 -           elements we keep around -->
>     1.55 -      <field name="mSizeOfAddressCache">3</field>

>    1.180 -            // we want to keep around the first mSizeOfAddressCache email address nodes
>    1.181 -            // don't forget that we have comma text nodes in there too so really we want to keep
>    1.182 -            // around cache size * 2 - 1.
>    1.183 -            var numItemsToPreserve = this.mSizeOfAddressCache * 2 - 1;
>    1.184 +            // We want to keep around the first maxAddressesBeforeMore email
>    1.185 +            // address nodes as well as any intervening comma nodes.
>    1.186 +            var numItemsToPreserve = this.maxAddressesBeforeMore * 2 - 1;
>    1.187              var numItemsInNode = aParentNode.childNodes.length;

Bottom line: Just replacing "maxAddressesBeforeMore" with "maxLinesBeforeMore" may be a safe choice, given that in the worst case each one address would occupy one line, or maybe a multiple of it to increase performance. A more fancy way might be to initialize it low, then check after the address nodes were filled up to the "more" limit how many addresses were used (but not if "more" was clicked), then update the current limit if it was too low. In this way, the maximum number of non-"more" nodes would be kept as cache for the next header and follow the desired "maxLinesBeforeMore" value roughly enough while still maintaining the benefits of bug 499989 to have reasonable performance with the "more" function.

Comment 4

[:aceman]

Great, you seem to be familiar with this stuff. Will you take the bug?
Do not expect a decision from me, I have no idea what the performance implications of this are.
From your explanation I would just change the line to this:
var numItemsToPreserve = this.maxLinesBeforeMore * 10 - 1
(I'd say at least 5 addresses fit on the line).
If that is not correct then I can't do more.

Comment 5

[rsx11m]

I'll give my ideas for the max-before-more on-the-fly solution a try.

Comment 6

[rsx11m]

Attachment: Proposed patch

This patch reintroduces the maxAddressesBeforeMore field, but no longer as a constant. It's initialized with an estimate of 1 and then updated whenever a larger number of addresses fit into the n lines before "more" has to be shown.

While I didn't notice much of a performance change even on a Pentium machine (apparently populating the nodes is more expensive than allocating or removing them), this now cleans up properly any address nodes beyond the "more" button once the view returns to the n-line display. I've monitored the cache size with the patch and it increases when going from a message with long e-mail addresses shown to one with more and shorted e-mail addresses and then stays there, but indeed removes additional nodes caused by clicking the "more" button on a view change. So, that works as expected and avoids carrying around a large memory footprint after clicking "more" on a message with 500+ addresses or so.

The patch was tested with various mailnews.headers.show_n_lines_before_more settings (=1,3,0) without encountering any apparent problems.

Comment 7

[Wayne Mery (:wsmwk)]

so this is a regression of bug 550487 ?

Comment 8

[rsx11m]

Technically yes, though a very late one. I'm a little reluctant to mark something with the "regression" keyword if it occurred a couple of release branches ago, but feel free to mark it so if you think it's appropriate to do so.

Blake: Any ETA for the review?

Comment 9

[Wayne Mery (:wsmwk)]

if it's pretty clear or definitely even if late, we should always mark with keyword and blocking.  It helps document the blood trail.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Just thought I'd weigh in on this bug, since I ran into the message-header address node caching stuff in bug 738374.

I'm curious - is there really a need anymore to cache these nodes?  Is there really such a performance benefit anymore?  Creating and appending nodes is remarkably cheap nowadays...

Comment 11

[rsx11m]

The effect certainly seems to be small, but the code is around (except for the missing maximum reinstated here), thus it can't hurt to do so either (unless it's causing issues with your bug).

Comment 12

[rsx11m]

Thinking more about it, even small actions which are "cheap" individually may sum up if you do them a few dozen of times, hence keeping around enough nodes to satisfy the default case (i.e., before the "more" is displayed) appears to be a sensible compromise. Thus, without having any data on a likely performance penalty one way or the other, I'd intuitively tend to stick with the limited caching of those nodes.

Comment 13

[rsx11m]

Doing some quick benchmarking on the Pentium machine with a stop watch, allowing clearChildNodes() to remove all nodes (thus effectively disabling the caching of nodes) gives me roughly a 0.5-1.0 second penalty over running it with the proposed patch for limited caching, with 3-5 lines visible when opening messages.

Comment 14

[rsx11m]

Comment on attachment 602504 [details] [diff] [review]
Proposed patch

Ping for review.

Blake: This has been sitting close to the top of your review queue for a month now, #2 last week. Can I hope that it's getting your attention some time soon?

Comment 15

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

rsx11m: You can always _hope_…  ;)

(I'll get to it tonight.)

Comment 16

[rsx11m]

Thanks, keeping fingers crossed...

Comment 17

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

Comment on attachment 602504 [details] [diff] [review]
Proposed patch

Yeah, I like it.  r=me.

(I would prefer it if it had some tests, but I can't think of a reasonable way to test it, so I'll let it slide. ;)

Comment 18

[rsx11m]

Great, thanks! Push for comm-central, please.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/3f939b4ac06b