493711 - need a better nsCOMArray with nsTArray-like API

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Arpad Borsos [:Swatinem]]

nsCOMArray is basically a ADDREF-ing wrapper to nsVoidArray and shadows its API.
It would be a lot better if there was a COMArray with an API similar to nsTArray<nsCOMPtr<T>>.

Quoting Jonas Sicking from the dev.platform thread:

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.

Comment 1

[Benjamin Smedberg]

There are two questions I think we need to answer:

1) Is this something we can rewrite automatically? Are the APIs compatible enough that we could automatically rewrite .Count to .Length, or are there fundamental differences in the API that would require reviewing every call site?

2) What are the codesize effects? If we could automatically rewrite for API differences, we could probably measure this pretty easily doing A-B builds.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

There are fundamental differences for some of the functions. Specifically around out-of-bounds behavior for the InsertObject(s)At/ReplaceObject(s)At functions. However I suspect these functions are rarely used, and even more rarely used using out-of-bounds indexes. But we have to manually verify.

If we use implementation strategy 3 from comment 0, I see no reason there needs to be any codesize differences. I would suggest we use that approach and then start experimenting with different implementation strategies, such as 4. That makes it easy to test the codesize differences.

Comment 3

[Arpad Borsos [:Swatinem]]

Automatic rewriting would introduce a lot of un/signed warnings, because Length() returns unsigned.

Also out-of-bounds safety needs to be taken care of.

Not quite sure if automatic rewriting would create more or less AddRefs, leading to problems. Manual rewriting would however catch a lot of cases where stuff like dealing with already_AddRefed could be simplified.

What needs to be rewritten manually is the use of COMArrays Enumeration API.

I believe porky should be able to generate an automatic patch for the simpler cases.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Late to the party, but why not use option (4) from comment #0 until we see performance/codesighs issues?  I don't see off the top of my head how codesighs would differ between (3) and (4), but I can certainly understand different performance due to smarter addref/unref.

> I believe porky should be able to generate an automatic patch for the simpler
> cases.

It can't rewrite templated types yet, but this is "easy" (in theory) to add.  A bit far down my queue atm, though.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #4)
> Late to the party, but why not use option (4) from comment #0 until we see
> performance/codesighs issues?  I don't see off the top of my head how 
> codesighs would differ between (3) and (4), but I can certainly understand
> different performance due to smarter addref/unref.

The problem with option (4) is that code increases are creeping up on us. Unless we do a giant patch that changes everything over in a single patch, which would let us to a before/after comparison.

The nice thing about (3) is that we can change all callsites function by function until we have the API that we want. Then it's just a matter of landing a small patch that changes just one class (nsCOMArray) which is low risk and easy to back out.


An alternative plan would be to gradually move callsites to nsTArray<nsCOMPtr<>> and ignore any size costs, noticed or not. Then experiment with different implementations and see if we can win back any losses that we may or may not have gotten. This does have the disadvantage that we'll potentially live with a size regression for a while, or possibly forever if the torch isn't carried all the way to the finish. Something generally frowned upon.


Ultimately I'm actually fine with any strategy and leave the decision to whomever steps up to do the work.

Comment 6

[Arpad Borsos [:Swatinem]]

There are already a few classes using TArray<COMPtr>: http://mxr.mozilla.org/mozilla-central/search?string=TArray%3C%23nsCOMPtr&regexp=on

I would suggest going with the typedef solution for the classes that are already using that style.
Maybe naming it mozilla::(Auto)COMArray<T> ? or is the namespace not enough to distinguish the old and the new type?

Someone more competent than me could then do (3) and see if it turns out to be a win.
Not sure if a win is measurable at all on such few cases. And (3) would need to be done anyway, even if it would turn out to be a loss afterward.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #6)
> There are already a few classes using TArray<COMPtr>:
> http://mxr.mozilla.org/mozilla-central/search?string=TArray%3C%23nsCOMPtr&regexp=on
> 
> I would suggest going with the typedef solution for the classes that are
> already using that style.
> Maybe naming it mozilla::(Auto)COMArray<T> ? or is the namespace not enough to
> distinguish the old and the new type?

I'm not sure what you are proposing here. Typedef what to what?

It's easy enough to make nsTArray<nsCOMPtr> be implemented using nsCOMArray. I think something like this is all that's needed:

NS_SPECIALIZE_TEMPLATE
class nsTArray<nsCOMPtr<T> > : public nsCOMArray
{
};

Of course, this will only compile if nsCOMArray uses the same API as nsTArray.

> Someone more competent than me could then do (3) and see if it turns out to be
> a win.
> Not sure if a win is measurable at all on such few cases. And (3) would need to
> be done anyway, even if it would turn out to be a loss afterward.

As long as someone does the work to one way or another give nsCOMArray the same API as nsTArray, I'm happy to play around with various implementations of nsCOMArray to see what gives best results.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oops, that should be

NS_SPECIALIZE_TEMPLATE
class nsTArray<nsCOMPtr<T> > : public nsCOMArray<T>
{
};

Comment 9

[Arpad Borsos [:Swatinem]]

Something like

namespace mozilla {
template<class T>
class COMArray : public nsTArray<nsCOMPtr<T> >
{}
} // namespace mozilla

should work, shouldn't it?

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think it'd be confusing to have two COMArrays.

But more importantly, what wouldthat buy us over moving callers to nsTArray?

Comment 11

[[e-mail]]

Attachment: Part 1 - switch mArray from nsVoidArray to nsTArray

Comment 12

[[e-mail]]

Attachment: Part 2 - provide an API resembling the nsTArray API

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Keep in mind that functions like ReplaceElementAt and InsertElementAt behaves differently between nsVoidArray and nsTArray for out-of-bounds and one-past-end accesses.

Is the first patch here keeping the nsVoidArray behavior for the *ObjectAt functions?

Comment 14

[[e-mail]]

(In reply to Jonas Sicking from comment #13)
> Keep in mind that functions like ReplaceElementAt and InsertElementAt
> behaves differently between nsVoidArray and nsTArray for out-of-bounds and
> one-past-end accesses.
> 
> Is the first patch here keeping the nsVoidArray behavior for the *ObjectAt
> functions?

It keeps the nsVoidArray behaviour for ReplaceElementAt. There is no special behaviour for InsertElementAt since nsTArray also accepts index == length.

Comment 15

[[e-mail]]

Oh, and I forgot to mention that Part 1 passed try (with randomorange).

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Cool. Though can we make ReplaceElementAt have nsTArray-like behavior. So that as we migrate code from *ObjectAt to *ElementAt we'll also migrate to the nsTArray behavior (and eventually can get rid of both the *ObjectAt functions as well as the nsVoidArray behavior)

Comment 17

[[e-mail]]

(In reply to [e-mail] from comment #15)
> Oh, and I forgot to mention that Part 1 passed try (with randomorange).
https://tbpl.mozilla.org/?tree=Try&rev=1ccbd9853f1d

(In reply to Jonas Sicking from comment #16)
> Cool. Though can we make ReplaceElementAt have nsTArray-like behavior. So
> that as we migrate code from *ObjectAt to *ElementAt we'll also migrate to
> the nsTArray behavior (and eventually can get rid of both the *ObjectAt
> functions as well as the nsVoidArray behavior)
Ah, that's a good point. I should probably write separate bodies for all the nsTArray-like methods instead of trying to share code with the old methods.

Comment 18

[[e-mail]]

Attachment: Part 1 - switch mArray from nsVoidArray to nsTArray

Added some range checking that nsTArray doesn't do but nsVoidArray does.
Made some base functions public because they didn't need to be protected.
Also now uses the EnsureLengthAtLeast API.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 693192 [details] [diff] [review]
Part 1 - switch mArray from nsVoidArray to nsTArray

Review of attachment 693192 [details] [diff] [review]:
-----------------------------------------------------------------

Generally looks great.

One thing occurred to me during review though. Is there a reason not to use mTArray<nsCOMPtr<nsISupports>> as the storage class? That should remove all of the manual refcounting.

Might be worth doing as a followup though.

Especially since it might be worth moving directly to making nsCOMArray<T> simply wrap a nsTArray<nsCOMPtr<T>> in which case lots of this code could go away.

::: xpcom/glue/nsCOMArray.cpp
@@ +6,5 @@
>  #include "nsCOMArray.h"
>  #include "nsCOMPtr.h"
>  
> +template<>
> +class nsTArrayElementTraits<nsISupports*>

Why is this class needed?

At the very least put a comment above the class saying that it's important that it's not moved outside of this compilation unit since it'll affect normal nsTArray<nsISupports*> instances.

@@ +133,5 @@
>  
>  bool
>  nsCOMArray_base::ReplaceObjectAt(nsISupports* aObject, int32_t aIndex)
>  {
> +    bool result = mArray.EnsureLengthAtLeast(aIndex + 1);

Hmm.. I guess this is where you are using the traits class?

@@ +206,5 @@
>  
> +    int32_t count = mArray.Length();
> +    if (count > aNewCount)
> +        RemoveObjectsAt(aNewCount, mArray.Length() - aNewCount);
> +    return mArray.SetLength(aNewCount);

And I guess this also uses the traits class?

Comment 20

[[e-mail]]

(In reply to Jonas Sicking from comment #19)
> One thing occurred to me during review though. Is there a reason not to use
> mTArray<nsCOMPtr<nsISupports>> as the storage class? That should remove all
> of the manual refcounting.
Yes, there is: nsTArray<nsCOMPtr<nsISupports>> is not reentrant, but nsCOMArray is, because it's in a consistent state whenever it releases a pointer.

> > +template<>
> > +class nsTArrayElementTraits<nsISupports*>
> Why is this class needed?
As you guessed below, I need the nsTArray to zero out new elements when its size increases.

> At the very least put a comment above the class saying that it's important
> that it's not moved outside of this compilation unit since it'll affect
> normal nsTArray<nsISupports*> instances.
Fair enough.

Comment 21

[[e-mail]]

Oops, part of part 2 crept into part 1...

Comment 22

[Mozilla RelEng Bot]

Try run for 797423183679 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=797423183679
Results (out of 19 total builds):
    failure: 19
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-797423183679

Comment 23

[[e-mail]]

Attachment: Part 1a - remove transitive dependency on nsVoidArray though nsCOMArray

Part 1b will remove the #include "nsVoidArray.h" from nsCOMArray.h

Comment 24

[[e-mail]]

Attachment: Part 1b - remove transitive dependency on nsVoidArray though nsCOMArray

Comment 25

[Mozilla RelEng Bot]

Try run for 797423183679 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=797423183679
Results (out of 20 total builds):
    exception: 1
    failure: 19
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-797423183679

Comment 26

[[e-mail]]

Attachment: Part 2 - provide an API resembling the nsTArray API

* Added start index parameter to IndexOf()
* Added Contains() (variant of IndexOf() == -1)
* Added s/Object/Element/ variants of nsCOMArray API
NOTE: nsTArray API takes the aIndex as the first, not the last argument
NOTE: nsTArray API uses unsigned indices, nsCOMArray API uses signed
NOTE: nsTArray API does not bounds check its indices, nsCOMArray API does
NOTE: no nsCOMArray_base::RemoveElement() added as the code is the same
* Added SwapElements() to exchange with another nsCOMArray of the same type
* Added Length()
* Added IsEmpty()
* Added TruncateLength()
NOTE: I didn't want to add array-lengthening primitives because of the dependency on the private traits. If we can remove all of the array-lengthening calls then we can get rid of those traits too.
* Added Capacity()

I also added a SwapElementsAt() that exchanges two elements.

Comment 27

[[e-mail]]

> * Added s/Object/Element/ variants of nsCOMArray API
...
> NOTE: I didn't want to add array-lengthening primitives because of the
> dependency on the private traits. If we can remove all of the
> array-lengthening calls then we can get rid of those traits too.
Which includes making ReplaceElementAt not lengthen the array first.

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You mean ReplaceObjectAt, right?

We can fix that by either moving exiating callers over to ReplaceElementAt after making sure they don't add things past the end of the array. Or by making ReplaceObjectAt expand the array size manually without using traits.

Comment 29

[[e-mail]]

(In reply to Jonas Sicking from comment #28)
> You mean ReplaceObjectAt, right?
I mean that, when I implemented ReplaceElementAt, I didn't copy ReplaceObjectAt's array-lengthening behaviour, which hasn't changed.

> We can fix that by either moving existing callers over to ReplaceElementAt
> after making sure they don't add things past the end of the array.
This is what I'm expecting to happen eventually.

Comment 31

[[e-mail]]

Attachment: Part 2 - provide an API resembling the nsTArray API

Fixed some typos and added a couple of extra useful methods.

Comment 32

[:aceman]

Will this make nsCOMArray NOT obsolete? https://developer.mozilla.org/en-US/docs/XPCOM_array_guide

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e9f71c463d9b