822695 - Use nsCOMArray

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: do it

I had discussed array containers for nsCOM objects just before writing this code, but somehow managed to still use std::vector.

Using nsCOMArray is fairly ugly, but does seem to be the best we have so far. With this change the text size of Telemetry.o goes from 84042 bytes to 83018 bytes.

https://tbpl.mozilla.org/?tree=Try&rev=f05b3a1e1f2d

Comment 1

[(no longer active)]

Comment on attachment 693447 [details] [diff] [review]
do it

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

Please use nsTArray<nsCOMPtr<...>> instead.  r=me with that.  (I don't think that you need to change any other part of the patch, but make sure that it builds ;-)

Comment 2

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

 > Please use nsTArray<nsCOMPtr<...>> instead.  r=me with that.  (I don't think
> that you need to change any other part of the patch, but make sure that it
> builds ;-)

It doesn't, why were you expecting two array implementations to have similar APIs? :-)

So, for size at least nsTArray is the worse:

std::vector: 84201 bytes
nsCOMPtr: 83177
nsTArray: 84399

Are you sure you want me to switch?

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: use nsTArray.

As noted on the previous comment, this is the worst one for code size :-(

Comment 4

[(no longer active)]

(In reply to comment #2)
>  > Please use nsTArray<nsCOMPtr<...>> instead.  r=me with that.  (I don't think
> > that you need to change any other part of the patch, but make sure that it
> > builds ;-)
> 
> It doesn't, why were you expecting two array implementations to have similar
> APIs? :-)
> 
> So, for size at least nsTArray is the worse:
> 
> std::vector: 84201 bytes
> nsCOMPtr: 83177
> nsTArray: 84399
> 
> Are you sure you want me to switch?

Did you happen to investigate why that is?  Is that because of more templated code?

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> > std::vector: 84201 bytes
> > nsCOMPtr: 83177
> > nsTArray: 84399
> > 
> > Are you sure you want me to switch?
> 
> Did you happen to investigate why that is?  Is that because of more
> templated code?

I have not looked at it, sorry.  There is probably some interesting missing optimization in nsTArray, but I could not find time to figure it out.

Comment 6

[(no longer active)]

(In reply to comment #5)
> > > std::vector: 84201 bytes
> > > nsCOMPtr: 83177
> > > nsTArray: 84399
> > > 
> > > Are you sure you want me to switch?
> > 
> > Did you happen to investigate why that is?  Is that because of more
> > templated code?
> 
> I have not looked at it, sorry.  There is probably some interesting missing
> optimization in nsTArray, but I could not find time to figure it out.

OK, you can go ahead with the original patch then, this is not important enough to waste too much time on it...

Comment 7

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/f196de2f77d6

Comment 8

[[e-mail]]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #2)
> > Please use nsTArray<nsCOMPtr<...>> instead.  r=me with that.  (I don't
> > think that you need to change any other part of the patch, but make sure
> > that it builds ;-)
> 
> It doesn't, why were you expecting two array implementations to have similar
> APIs? :-)

That's bug 493711, waiting for review from sicking.

Comment 9

[Ed Morley [:emorley]]

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