394691 - Make nsTArrayElementTraits not do default initialization

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsAutoBuffer should inherit from nsAutoTArray and take a number of initial elements as a constructor parameter. These elements should be allocated in the array without doing any construction. All current users of nsAutoBuffer should be updated to this syntax.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

If we change nsAutoBuffer to be implemented in terms of nsAutoTArray,
with a constructor that takes the size as an argument, we have no way
of knowing if the resize nsAutoTArray's buffer needed in the constructor
succeeds or not, as we can't return an error code from the
constructor. So the solution is to add a function
AddUnInitializedElements(n) to nsAutoTArray, and remove nsAutoBuffer
entirely. All uses of nsAutoBuffer need to be changed to nsAutoTArray.

Comment 2

[Stuart Parmenter]

We've discussed adding some kind of AddUnInitializedElements to nsAutoTArray, but we decided against it for various reasons.

nsAutoBuffer already takes a size in its constructor.  I'm not sure what the problem you're describing is.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

nsAutoBuffer doesn't take a size parameter to it's constructor, it takes a template argument which is the size of its stack buffer. I think the idea was to refactor this so that nsAutoBuffer uses nsAutoTArray, but the problem with nsAutoTArray is that it calls the constructor of elements it constructs, which is a performance problem.

So... if we change nsAutoBuffer to be implemented in terms of nsAutoTArray, with a constructor that calls nsTArray::EnsureCapacity() to ensure that the buffer is the correct size, what happens if the EnsureCapacity() fails? We can't return failure in the constructor, so we need to move the EnsureCapacity() out to another function in which we can check for failure... much like the original class. So we could keep nsAutoBuffer, and make it inherit from nsAutoTArray, but have a no-argument constructor. Then we'd change EnsureElemCapacity() and AddElemCapacity() to add uninitialized elements to it. 

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> We've discussed adding some kind of AddUnInitializedElements to nsAutoTArray,
> but we decided against it for various reasons.

You argued against it on the grounds that people would have to remember to use that method. It seems to me that having the separate nsAutoBuffer type has no advantage there --- people have to remember to use the different type instead. Were there any other reasons?

I thought having a separate (sub)type would allow us to have a special constructor that automatically adds a number of uninitialized elements ... but I didn't think about the problem that constructors can't return error codes. So I now don't have a good reason to use a separate type.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ok, there's a much better way to fix this.

nsTArrayElementTraits does this:

    static inline void Construct(E *e) {
      new (static_cast<void *>(e)) E();
    }

This triggers C++ default initialization for E. In particular POD types get zeroed out.

Instead we can write:

    static inline void Construct(E *e) {
      new (static_cast<void *>(e)) E;
    }

Then C++ default initialization will not be performed. Types with a default constructor will still get that constructor called. POD types will not be initialized. This matches the semantics of regular C/C++ arrays and is exactly what we want here.

We need to audit code and/or do some testing to make sure this is safe, but we should definitely do it. Fortunately the only nsTArray methods that call this function are AppendElements, InsertElementsAt, and the 1-arg version of InsertElementAt.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1

Replaces uses of nsAutoBuffer with nsTAutoArray, and changed the Construct() function to allow default C++ initialization.

I've run Mochitests and reftests over it, and nothing seems broken.

Comment 7

[Reed Loden [:reed]]

Comment on attachment 280700 [details] [diff] [review]
Patch v1

Please use `cvs diff -u9p` to create patches so that the patches are unidiff, have more context, and show surrounding functions. Context diffs are so hard to read.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #7)
> (From update of attachment 280700 [details] [diff] [review])
> Please use `cvs diff -u9p` to create patches so that the patches are unidiff,
> have more context, and show surrounding functions. Context diffs are so hard to
> read.
> 

Sorry about that, I was following the guidelines from http://www.mozilla.org/hacking/life-cycle.html which said to do a 'cvs diff -u8pN'. I'll redo the patch...

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Same as the previous patch, but -u9p

This is the same patch as the previous, but produced -u8p, as opposed to the old one which was -u8pN.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In some places you're including nsAutoTArray.h, but that file doesn't exist --- should be nsTArray.h.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch V2 - Mac #includes fixed.

Previous patch with Mac #include's fixed.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3 with another Mac compile err fix

Previous patch with mac compile error removed. Should compile on mac now as well, please review.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch with more mac fixes

Here's the patch with more fixes for the mac... Can someone with a Mac try compiling this for me please?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

-    nsAutoBuffer<UniChar, 32> buffer;
+    nsTAutoArray<UniChar, 32> buffer;

nsAutoTArray

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: The "I need a mac" patch

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This patch has a number of "#include "nsAutoTArray.h" ...

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Darn, I screwed up. I know how this happened, I'll change my process so that it won't happen again. Regenerating patch...

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch with includes fixed again.

Sorry for the mixups with the previous patches!

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 281128 [details] [diff] [review]
Patch with includes fixed again.

This builds!

I'll sr the mass changed and let's ask Benjamin for review of the core change to not initialize POD element types.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 281128 [details] [diff] [review]
Patch with includes fixed again.

This will help performance ... no idea how much.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> old one which was -u8pN.

Except it wasn't. It had none of those flags, if you look at the actual diff in 
https://bugzilla.mozilla.org/attachment.cgi?id=280700

Comment 22

[Boris Zbarsky [:bzbarsky]]

Please get a bug filed on GetDictionaryStringValue not checking whether its SetLength() call succeeded?  Same for a number of other callers in this patch.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 281128 [details] [diff] [review]
Patch with includes fixed again.

a=bzbarsky, though the nsTArray code could use a comment so that no one readds the parens.  Don't forget to cvs remove nsAutoBuffer!

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in. I'll remove nsAutoBuffer after I'm sure it won't need to be reverted.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Doesn't seem to have made a noticeable performance difference. Oh well.

nsAutoBuffer.h removed.

Comment 26

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #22)
> Please get a bug filed on GetDictionaryStringValue not checking whether its
> SetLength() call succeeded?  Same for a number of other callers in this patch.
> 

I've added Bug 396647 for this.

Comment 27

[Walter Meinl]

I found a leftover of nsAutoBuffer
http://mxr.mozilla.org/seamonkey/source/gfx/src/windows/nsFontMetricsWin.cpp#646
intentional?

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Not intentional, but not a problem since that code is obsolete and will never be used again.

Comment 29

[[e-mail]]

(In reply to comment #5)
> Ok, there's a much better way to fix this.
> 
> nsTArrayElementTraits does this:
> 
>     static inline void Construct(E *e) {
>       new (static_cast<void *>(e)) E();
>     }
> 
> This triggers C++ default initialization for E. In particular POD types get
> zeroed out.
> 
> Instead we can write:
> 
>     static inline void Construct(E *e) {
>       new (static_cast<void *>(e)) E;
>     }
> 
> Then C++ default initialization will not be performed.

FYI this was one of VC6's bugs - neither syntax would initialise POD types.

Possibly due to a coding oversight at the time which was not noticed because of the zeroing, nsTextFrameThebes used to assume zeroing out of POD types; is it possible that other users of nsTArray have inadvertently made that assumption?

Comment 30

[Peter Van der Beken [:peterv]]

Comment on attachment 281128 [details] [diff] [review]
Patch with includes fixed again.

>Index: intl/locale/src/nsLocaleService.cpp
>===================================================================

>     int size = ::CFStringGetLength(userLocaleStr);
>-    if (buffer.EnsureElemCapacity(size))
>+    if (buffer.SetLength(size))
>     {
>         CFRange range = ::CFRangeMake(0, size);
>-        ::CFStringGetCharacters(userLocaleStr, range, buffer.get());
>-        buffer.get()[size] = 0;
>+        ::CFStringGetCharacters(userLocaleStr, range, buffer.Elements());
>+        buffer[size] = 0;

This asserts:

###!!! ASSERTION: invalid array index: 'i < Length()', file ../../../dist/include/xpcom/nsTArray.h, line 317

Maybe remove that line and pass size to the nsAutoString constructor:

>         // Convert the locale string to the format that Mozilla expects
>-        nsAutoString xpLocale(buffer.get());
>+        nsAutoString xpLocale(buffer.Elements());

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> is it possible that other users of nsTArray have inadvertently made that
> assumption?

It's possible. Everyone I talked to was surprised that nsTArray was initializing PODs and not behaving like a regular array, so I don't think there'll be much of that. We ran all tests and didn't find anything.