Closed Bug 483764 Opened 12 years ago Closed 12 years ago

Switch database caches from nsVoidArray to nsTArray

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: standard8, Assigned: standard8)

Details

(Keywords: memory-footprint, perf)

Attachments

(1 file, 1 obsolete file)

Attached patch The fix (obsolete) — Splinter Review
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.
Attachment #367747 - Flags: superreview?(bienvenu)
Attachment #367747 - Flags: review?(neil)
why would you use a stack-based variable in a static object? The static object won't be allocated on the stack.
Attachment #367747 - Flags: superreview?(bienvenu) → superreview+
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.
Attachment #367747 - Flags: review?(neil) → review-
Comment on attachment 367747 [details] [diff] [review]
The fix

#developers (well, bz) said that static T arrays were a bad idea.
(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*.
Attached patch The fix v2Splinter Review
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.
Attachment #367747 - Attachment is obsolete: true
Attachment #368228 - Flags: superreview?(bienvenu)
Attachment #368228 - Flags: review?(neil)
(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...
Attachment #368228 - Flags: review?(neil) → review+
Attachment #368228 - Flags: superreview?(bienvenu) → superreview+
Checked in: http://hg.mozilla.org/comm-central/rev/a4dcc4160c3f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.