nsCOMArray<T> vs. nsTArray<nsCOMPtr<T> >
Arpad Borsos
------------------------
While working on nsVoidArray, I came across a few situation where it
was not really clear which one I should use.
So lets summarize some of the differences/similarities of the two.
The goal of both variants is to hold a Reference to the COM objects
they contain. So basically Addref on add and Release on remove.
The TArray+COMPtr variant has the added benefit of working together
with already_AddRefed, while COMArray does not.
COMArray mirrors the VoidArrays sorting and enumeration API, whereas
the TArray approach offers a much clearer API and can also take
advantage of specialized nsDefaultComparator variants.
The TArray variant also offers a nicer replacement API which takes
advantage of COMPtrs assignment operator, again working nicely
together with already_AddRefed.
Regarding out-of-bounds safety:
Both COMArray and TArrays Object/ElementAt are not out-of-bounds safe.
COMArrays InsertElement(s)At can act as Append when Index = Length but
fails when Index > Length. With TArray, I'm not quite sure.
COMArrays ReplaceElementAt can implicitly add Elements to the Array,
whereas using TArray+assignment operator does not.
Some people have raised concerns about codesize, memory usage and
performance so that needs to be evaluated.
As a quite new developer, I can say that I find the TArray+COMPtr API
easier to work with.
Comments and suggestions welcome :-)
------------------------
As for memory usage:
COMArray: 1pointer to VoidArray ( 1pointer to mImpl [4bytes for mBits
+ 4bytes for mCount + N*1pointer]) = depending on architecture 16bytes
+ N*4bytes or 24bytes + N*8bytes
TArray: 1pointer to mHdr (4bytes for mLength + 4bytes for mCapacity) +
N*1pointer (mRawPtr of COMPtr) = 12bytes + N*4bytes or 16bytes +
N*8bytes
Is this correct or have I done something wrong?
Boris Zbarsky
> Copied from mdt.xpcom:
As I said there:
One option would be to change the nsCOMArray API, of course. Have to be
pretty careful, but it's a thought.
-Boris
Arpad Borsos
Btw, I'm weakly in favor of keeping existing nsCOMArray users using
nsCOMArray.
But make nsCOMArray be based on nsTArray. The reason is that I suspect
that we
can make nsCOMArray<T> produce more optimized code than
nsTArray<nsCOMPtr<T>>.
Though, I would also be ok with typedef-ing nsCOMArray<T> to
nsTArray<nsCOMPtr<T>>. If it turns out that we can optimize the
implementation,
we can always do that later.
Boris Zbarsky
> But make nsCOMArray be based on nsTArray. The reason is that I suspect
> that we
> can make nsCOMArray<T> produce more optimized code than
> nsTArray<nsCOMPtr<T>>.
That would change the API as I proposed, yes. Presumably using
nsTArray<nsCOMPtr<nsISupports>> or some such here.
> Though, I would also be ok with typedef-ing nsCOMArray<T> to
> nsTArray<nsCOMPtr<T>>. If it turns out that we can optimize the
> implementation,
> we can always do that later.
This would have nontrivial codesize impact, I suspect. You can check
this locally, though...
-Boris
Benjamin Smedberg
> Btw, I'm weakly in favor of keeping existing nsCOMArray users using
> nsCOMArray.
> But make nsCOMArray be based on nsTArray. The reason is that I suspect
> that we
> can make nsCOMArray<T> produce more optimized code than
> nsTArray<nsCOMPtr<T>>.
I tend to agree with sicking, at least for the moment. Let's keep using
nsCOMArray (and not use nsTArray<nsCOMPtr<T>>).
I certainly would not want a general switch to nsTArray<nsCOMPtr<T>> until
we have codesize numbers.
--BDS
Zack Weinberg
It ought to be possible to make them indistinguishable, by appropriate
partial specializations.
zw
Jonas Sicking
> Arpad Borsos wrote:
>> But make nsCOMArray be based on nsTArray. The reason is that I suspect
>> that we
>> can make nsCOMArray<T> produce more optimized code than
>> nsTArray<nsCOMPtr<T>>.
>
> That would change the API as I proposed, yes. Presumably using
> nsTArray<nsCOMPtr<nsISupports>> or some such here.
I absolutely 100% think that we should change the API. So that the
nsCOMArray API matches that of nsTArray.
nsVoidArray should ideally die and Arpad is making amazing progress on
this. Along with killing nsVoidArray we should kill the API that came
with it :)
The options that I see are these:
1. Keep nsCOMArray as is, with current API and current non-nsTArray
implementation
2. Keep nsCOMArray as is, with current API, but one that forwards to an
nsTArray implementation (which means adding code to make up for the
differences in API)
3. Change nsCOMArray to use the nsTArray API, and use an nsTArray
implementation
4. Change nsCOMArray to be a typedef of nsTArray<nsCOMPtr<T>>
I definitely do not think 1 or 2 are good ideas.
Between 3 and 4 I would choose 4 if the difference in code size is not
too big, and 3 otherwise. I would say that a small increase in codesize
is ok.
If we go with 4 for now, we can always play around with 3 to find an
implementation that is smaller/faster/more awesome.
/ Jonas
PS. If you wanna kill nsSupportsArray too while you're at it that would
be rockin'. Though there might be frozen interfaces that use it, which
might make it impossible to do for now :(
Neil
>Some people have raised concerns about codesize, memory usage and performance so that needs to be evaluated.
>
>As a quite new developer, I can say that I find the TArray+COMPtr API easier to work with.
>Comments and suggestions welcome :-)
>
>
One issue is that nsTArray is much more generic than nsCOMArray. Indeed,
nsCOMArray_base does all the work for the template, which only exists to
provide type-safety. m My suggestion is therefore to make
nsCOMArray_base's mArray an nsTArray<nsISupports*>, but not to change
nsCOMArray's API.
>As for memory usage:
>COMArray: 1pointer to VoidArray ( 1pointer to mImpl [4bytes for mBits + 4bytes for mCount + N*1pointer]) = depending on architecture 16bytes + N*4bytes or 24bytes + N*8bytes
>
>
No, it's a member void array, so it's the same size as an nsTArray.
>TArray: 1pointer to mHdr (4bytes for mLength + 4bytes for mCapacity) + N*1pointer (mRawPtr of COMPtr) = 12bytes + N*4bytes or 16bytes + N*8bytes
>
>
Also one mHdr for every unique type templated per module (so perhaps
less significant for libxul).
P.S. Any chance of working on mailnews some time? ;-)
--
Warning: May contain traces of nuts.
Arpad Borsos
You are right.
> P.S. Any chance of working on mailnews some time? ;-)
Filed bug 485693 for comm-central.
> 2. Keep nsCOMArray as is, with current API, but one that forwards to an
> nsTArray implementation (which means adding code to make up for the
> differences in API)
> 3. Change nsCOMArray to use the nsTArray API, and use an nsTArray
> implementation
> 4. Change nsCOMArray to be a typedef of nsTArray<nsCOMPtr<T>>
3 and 4 are basically the same on the users side.
I believe 2 needs to be done when rewriting the users to the new API,
so there are old API and new API users side-by-side.
2 would then mean the following:
make nsCOMArray privately subclass nsTArray<nsCOMPtr<nsISupports> >
and reimplement the TArray methods to translate from T to nsISupports
and back?
make nsCOMArrays old API use the TArray API in the background, again
translating from T to nsISupports and back?
After that, the old COMArray API could be dropped
> PS. If you wanna kill nsSupportsArray too while you're at it that would
> be rockin'. Though there might be frozen interfaces that use it, which
> might make it impossible to do for now :(
You do mean nsISupportsArray (notice the I ), right?
I've already looked at those bugs. Looks interesting. I may get to
those when I'm finished with nsVoidArray.
Boris Zbarsky
> I believe 2 needs to be done when rewriting the users to the new API,
> so there are old API and new API users side-by-side.
That might be hard because the two APIs have methods with identical
names but different signatures (e.g. IndexOf(), operator[]). You might
be able to overload if they differ on things other than return type. If
this works, great.
>> PS. If you wanna kill nsSupportsArray too while you're at it that would
>> be rockin'. Though there might be frozen interfaces that use it, which
>> might make it impossible to do for now :(
>
> You do mean nsISupportsArray (notice the I ), right?
That too, but nsSupportsArray is the concrete class implementing it.
I'm pretty sure nsISupportsArray is used in some frozen interfaces
(hello, nsIWindowWatcher!).
-Boris
Arpad Borsos
What kind of transition strategy would you propose?
Boris Zbarsky
> I see.
> What kind of transition strategy would you propose?
One obvious option is to create a new class based on
nsTArray<nsCOMPtr<T>>, and switch consumers to it one by one. Then
remove nsCOMArray when there are no more consumers. If we want the new
class to be named nsCOMArray, do a global rename afterward.
But if you can just expose the new API on nsCOMArray, then go for it. I
was just pointing out that it might not work; it's worth checking
whether it actually does.
-Boris
Jonas Sicking
> Arpad Borsos wrote:
>
>> Some people have raised concerns about codesize, memory usage and
>> performance so that needs to be evaluated.
>>
>> As a quite new developer, I can say that I find the TArray+COMPtr API
>> easier to work with.
>> Comments and suggestions welcome :-)
>>
>>
> One issue is that nsTArray is much more generic than nsCOMArray. Indeed,
> nsCOMArray_base does all the work for the template, which only exists to
> provide type-safety. m My suggestion is therefore to make
> nsCOMArray_base's mArray an nsTArray<nsISupports*>, but not to change
> nsCOMArray's API.
I think it's a bad idea to have two Array APIs that behave almost the
same way. For example having
myCOMArray.InsertObjectAt(object, index)
myTArray.InsertElementAt(index, object)
Where first off the two arguments are reversed, second where one is safe
for out-of-bounds, but the other is not. The situation is even worse for
ReplaceElement(s)At since there out-of-bounds is not just safe on
nsCOMArray, but actually functional.
So no matter what implementation we use, I do think we want to change
the API on nsCOMArray.
That said, I do agree that it's important that we save cycles and bytes
where it makes sense by not using a bloated implementation of
nsCOMArray. However that is IMHO an orthogonal issue.
/ Jonas
Zack Weinberg
>
> So no matter what implementation we use, I do think we want to change
> the API on nsCOMArray.
If we're changing APIs anyway, why not jump all the way to std::vector?
zw
Jonas Sicking
I am also a bit short on ideas for transition strategies.
Other than what Boris proposes, here one other:
Find all the functions that have a different API between the two
classes. In many cases the difference is just in the name, i.e.
AppendElement vs. AppendObject. In other cases the differences are
bigger, such as ReplaceObjectAt vs. ReplaceElementsAt. It's very likely
that the latter group is not very heavily used.
Then change functions one at a time over to the new API. For the first
group that means just doing a search and replace. For the latter group
it's a bit more work, but shouldn't be too bad since there should be
very few consumers.
Does that make sense?
/ Jonas
Benjamin Smedberg
According to authorities who know it better than I, the STL in general
doesn't deal well or interoperably when you have exceptions turned off.
--BDS
Arpad Borsos
>
> According to authorities who know it better than I, the STL in general
> doesn't deal well or interoperably when you have exceptions turned off.
Wasn't there a proposal to make gecko use exceptions some time ago?
What happened to that?
Also as a side note because I came over it today: You can create a
nsISimpleEnumerator from a nsCOMArray. This also needs to be
considered when working up a new api.
Robert O'Callahan
>>> If we're changing APIs anyway, why not jump all the way to std::vector?
>> According to authorities who know it better than I, the STL in general
>> doesn't deal well or interoperably when you have exceptions turned off.
>
> Wasn't there a proposal to make gecko use exceptions some time ago?
> What happened to that?
Initial experiments suggested it would be a big code-size increase.
Rob
dbradley
I thought the issues were more around when exceptions were turned on.
Things like what happens if a constructor or destructor of a member of
the container throws when the container is being initialized or
destroyed. This discussion brought back memories of http://www.gotw.ca/gotw/008.htm
I've never quite understood the need for Mozilla to provide classes so
different than what Standard C++ provides. I know some of it is the
age and poor template support of compilers. And there are valid
performance and portability issues. But some of the new classes seem
to be needlessly different. I can understand creating clones because
of performance or portability issues, but why diverge so far? I don't
work solely on Mozilla code and it's just a pain to have to throw out
all the patterns I've learned when I work with nsTArray and the like.
David