483764 - Switch database caches from nsVoidArray to nsTArray

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

Attachment: The fix

I was looking at the database cache code recently and realised that int nsMsgDatabase::FindInCache(nsMsgDatabase* pMessageDB) was actually just the same as nsVoidArray::IndexOf, but slower (due to it being implemented outside of nsVoidArray).

At the same time as fixing that I thought we might as well change to nsTArray at the same time and drop casts etc.

There's two main changes I've also made:

- The static variable is now created on the stack, not dynamically.
- I've set an initial array capacity of 20 for the nsMsgDatabase, and 3 for nsAddrDatabase. This is set on the stack, so we'll avoid re-allocs until we reach that limit for open databases.

I think 3 is reasonable for the address book - two by default, plus a possible OS one.

20 is a bit of a guess, but given we don't always open all folders at the same time, it should give us a bit of room for re-allocs.

With both the numbers I'm open to suggestions. It won't improve perf that much, but it will should some initial reallocs at least.

I've had this patch in my stack the last few days and not noticed any problems yet.

Comment 1

[David :Bienvenu]

why would you use a stack-based variable in a static object? The static object won't be allocated on the stack.

Comment 2

[David :Bienvenu]

Comment on attachment 367747 [details] [diff] [review]
The fix

modulo the fact that the var won't be allocated from the stack, this looks ok to me.

Comment 3

[[e-mail]]

Comment on attachment 367747 [details] [diff] [review]
The fix

#developers (well, bz) said that static T arrays were a bad idea.

Comment 4

[Mark Banner (:standard8)]

(In reply to comment #3)
> (From update of attachment 367747 [details] [diff] [review])
> #developers (well, bz) said that static T arrays were a bad idea.

Sigh, it'd be nice if they could actually *document* that somewhere and explain *why*.

Comment 5

[Mark Banner (:standard8)]

Attachment: The fix v2

Made m_dbCache a pointer again, though I'm still using nsAutoTArray in the new statement so that we can preallocate buffer size and avoid some reallocs.

Comment 6

[[e-mail]]

(In reply to comment #4)
> it'd be nice if they could actually *document* that somewhere and explain *why*.
I'm sure I saw something referring to destructors of static variables which apparently on the Mac can run after the dependent library has unloaded...

Comment 7

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/a4dcc4160c3f