replace nsUint8Array with nsTArray<PRUint8>

RESOLVED FIXED in mozilla1.9beta3

Status

RESOLVED FIXED
15 years ago
5 years ago

People

(Reporter: Biesinger, Assigned: neil)

Tracking

Trunk
mozilla1.9beta3
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 5 obsolete attachments)

there's no need for mailnews to invent own data structures when xpcom already
provides them.
Assignee: cbiesinger → cst
Created attachment 157774 [details] [diff] [review]
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
We should really add an easy way to SetValueAt (or fix operator[] to allow it on
the LHS) too for nsValueArray.
Status: NEW → ASSIGNED
Created attachment 157775 [details]
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
Allowing values up to 1024 got rid of the aValue asserts, but didn't solve the
problem in the attached screenshot.
(Assignee)

Comment 5

14 years ago
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.
Created attachment 157891 [details] [diff] [review]
Patch to remove nsUint8Array

Neil, thanks - that fixed it.
Attachment #157774 - Attachment is obsolete: true
Attachment #157775 - Attachment is obsolete: true
Created attachment 157892 [details] [diff] [review]
Patch to remove nsUint8Array

trivial changes
Attachment #157891 - Attachment is obsolete: true
Attachment #157892 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #157892 - Flags: review?(bienvenu)
(Assignee)

Comment 8

14 years ago
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.
Attachment #157892 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
(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?
Attachment #157892 - Attachment is obsolete: true
Attachment #157892 - Flags: review?(bienvenu)

Comment 10

14 years ago
it's a performance thing, but we don't want to reduce performance.
(Assignee)

Comment 11

14 years ago
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.
(Assignee)

Comment 12

14 years ago
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.
Product: MailNews → Core
Whiteboard: blocked → helpwanted
Assignee: cst → sspitzer
Target Milestone: --- → Future
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
(Assignee)

Updated

11 years ago
Summary: replace nsUint8Array with nsValueArray → replace nsUint8Array with nsTArray<PRUint8>
(Assignee)

Comment 14

11 years ago
Created attachment 295013 [details] [diff] [review]
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 :-)
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #295013 - Flags: superreview?(bienvenu)
Attachment #295013 - Flags: review?(bienvenu)
(Assignee)

Comment 15

11 years ago
Oh, and I decided that the SetAtGrow wasn't necessary as there must already be a key for that index.
(Assignee)

Comment 16

11 years ago
Created attachment 295377 [details] [diff] [review]
Updated for bitrot

I also accidentally deleted a hunk from a Makefile.in in the last patch.
Attachment #295013 - Attachment is obsolete: true
Attachment #295377 - Flags: superreview?(bienvenu)
Attachment #295377 - Flags: review?(bienvenu)
Attachment #295013 - Flags: superreview?(bienvenu)
Attachment #295013 - Flags: review?(bienvenu)

Comment 17

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

cool, thx, Neil.
Attachment #295377 - Flags: superreview?(bienvenu)
Attachment #295377 - Flags: superreview+
Attachment #295377 - Flags: review?(bienvenu)
Attachment #295377 - Flags: review+
(Assignee)

Comment 18

11 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: helpwanted
Target Milestone: Future → mozilla1.9 M11
Depends on: 412268
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.