250811 - replace nsUint8Array with nsTArray<PRUint8>

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

there's no need for mailnews to invent own data structures when xpcom already
provides them.

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: preliminary patch

<CTho> ###!!! ASSERTION: Lossy value array detected.  Report a higher maximum
upon construction!: '*((PRUint8*)&mValueArray[aIndex * mBytesPerValue]) ==
aValue', file  
<CTho> d:/cvs-1.11.5/mozilla/xpcom/ds/nsValueArray.cpp, line 191 
<bz> CTho: er... what's aValue there?
<CTho> aValue = 258 
<bz> CTho: and this was being stored in a PRUint8?
<CTho> apparently 
<bz> CTho: smoooth

The caller:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/src/nsMsgDBView.cpp&mark=4486-4490#4486

bienvenu, neil, mscott are apparently guilty for this part of the code

Comment 2

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

We should really add an easy way to SetValueAt (or fix operator[] to allow it on
the LHS) too for nsValueArray.

Comment 3

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Bustage likely caused by my patch

I don't use threads normally so this may occur normally, but I doubt it.  Most
subjects appear blank in the list.  Clicking the down-arrow next to a subject
produces:

###!!! ASSERTION: couldn't find thread: 'PR_FALSE', file
d:/cvs-1.11.5/mozilla/mailnews/base/src/nsMsgDBView.cpp, line 3928

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Allowing values up to 1024 got rid of the aValue asserts, but didn't solve the
problem in the attached screenshot.

Comment 5

[[e-mail]]

OK, the most likely cause of your issue is that InsertAt's parameters are
(position, value) but InsertValueAt is expecting (value, position).

nsValueArray could really do with a ReplaceValueAt method.

nsMsgDBView::ReverseThreads doesn't need to allocate its arrays on the heap i.e.
use nsValueArray newLevelArray(255, m_levels.Count()); and change the ->s to .s

nsMsgDBView::ReverseThreads uses a loop to copy a portion of one array to
another array which is a bit inefficient but offhand I can't see a better way.

Comment 6

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch to remove nsUint8Array

Neil, thanks - that fixed it.

Comment 7

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch to remove nsUint8Array

trivial changes

Comment 8

[[e-mail]]

Comment on attachment 157892 [details] [diff] [review]
Patch to remove nsUint8Array

>+  :m_levels(255,0)
You don't need the 0 as it's the default value for that parameter.

>-    nsUint8Array *newLevelArray = new nsUint8Array;
>+    nsValueArray *newLevelArray = new nsValueArray(255,0);
The second parameter needs to be m_levels.GetSize() to preallocate.
Also this variable could be converted from a pointer to a local i.e. allocated
on the stack rather than on the heap.

>+            //XXX cst - we need a SetValueAt for nsValueArray
Maybe you should get that fixed first?

>-      while ((int32) threadIndex < m_levels.GetSize() && m_levels[threadIndex] != 0);
>+      while ((int32) threadIndex < m_levels.Count() && m_levels[threadIndex] != 0);
Count() returns an unsigned while for some odd reason GetSize() returns a
signed, so you'll have to fix up some signed/unsigned mismatches. Hopefully
this will be a case of removing erroneous casts :-)

>+      m_levels.AppendValue(PRUint8(levelToAdd));
You don't need these casts.

>+        m_prevLevels.Clear();
>+        m_prevLevels = m_levels;
You don't need to clear before copy.

>-          m_levels.SetAtGrow(index, 0);
You didn't replace this line.

Comment 9

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #8)
> >+            //XXX cst - we need a SetValueAt for nsValueArray
> Maybe you should get that fixed first?
ok, see bug blocking this one

> >-          m_levels.SetAtGrow(index, 0);
> You didn't replace this line.
nsValueArray doesn't seem to have an equivalent.  It's a performance thing, not
a functionality thing, right?

Comment 10

[David :Bienvenu]

it's a performance thing, but we don't want to reduce performance.

Comment 11

[[e-mail]]

SetAtGrow allows the array to grow, SetAt does not. nsValueArray doesn't have an
equivalent, but your new ReplaceValueAt should be able to handle it.

Comment 12

[[e-mail]]

Comment on attachment 157892 [details] [diff] [review]
Patch to remove nsUint8Array

>-    newLevelArray->SetSize(m_levels.GetSize());
Actually what I said last time I commented on the patch was wrong in a
different way, but this code expects to be able to preallocate space so that it
can be set later.

>-  m_levels.SetSize(numThreads);
And here the code expects to be able to change the count too.

>-          m_levels.SetAtGrow(index, 0);
This would seem to be a false alarm - index is always valid, so it's the same
as SetAt.

Comment 13

[(not reading, please use [e-mail] instead)]

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 14

[[e-mail]]

Attachment: Proposed patch

I changed newLevelArray to be stack-allocated instead of heap-allocated (the actual array elements are still heap-allocated) and swapped instead of copying.
CollapseByIndex now removes all the elements at once.
I tweaked nsMsgGroupView::OnNewHeader, let me know if you don't like it.

This even fixes a signed/unsigned mismatch warning :-)

Comment 15

[[e-mail]]

Oh, and I decided that the SetAtGrow wasn't necessary as there must already be a key for that index.

Comment 16

[[e-mail]]

Attachment: Updated for bitrot

I also accidentally deleted a hunk from a Makefile.in in the last patch.

Comment 17

[David :Bienvenu]

Comment on attachment 295377 [details] [diff] [review]
Updated for bitrot

cool, thx, Neil.

Comment 18

[[e-mail]]

Fix checked in.