Make nsTArrayElementTraits not do default initialization

RESOLVED FIXED

Status

()

RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: roc, Assigned: cpearce)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

11 years ago
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

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

Comment 3

11 years ago
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. 
> 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.
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.
Summary: Make nsAutoBuffer inherit from nsAutoTArray → Make nsTArrayElementTraits not do default initialization
(Assignee)

Comment 6

11 years ago
Created attachment 280700 [details] [diff] [review]
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.
Attachment #280700 - Flags: review?(benjamin)
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.
(Assignee)

Comment 8

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

Comment 9

11 years ago
Created attachment 280704 [details] [diff] [review]
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.
Attachment #280700 - Attachment is obsolete: true
Attachment #280700 - Flags: review?(benjamin)
In some places you're including nsAutoTArray.h, but that file doesn't exist --- should be nsTArray.h.
(Assignee)

Comment 11

11 years ago
Created attachment 280801 [details] [diff] [review]
Patch V2 - Mac #includes fixed.

Previous patch with Mac #include's fixed.
Attachment #280704 - Attachment is obsolete: true
(Assignee)

Comment 12

11 years ago
Created attachment 280818 [details] [diff] [review]
Patch v3 with another Mac compile err fix

Previous patch with mac compile error removed. Should compile on mac now as well, please review.
Attachment #280801 - Attachment is obsolete: true
Attachment #280818 - Flags: review?(benjamin)
(Assignee)

Comment 13

11 years ago
Created attachment 280843 [details] [diff] [review]
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?
Attachment #280818 - Attachment is obsolete: true
Attachment #280818 - Flags: review?(benjamin)
-    nsAutoBuffer<UniChar, 32> buffer;
+    nsTAutoArray<UniChar, 32> buffer;

nsAutoTArray
(Assignee)

Comment 15

11 years ago
Created attachment 280848 [details] [diff] [review]
The "I need a mac" patch
Attachment #280843 - Attachment is obsolete: true
This patch has a number of "#include "nsAutoTArray.h" ...
(Assignee)

Comment 17

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

Comment 18

11 years ago
Created attachment 281128 [details] [diff] [review]
Patch with includes fixed again.

Sorry for the mixups with the previous patches!
Attachment #280848 - Attachment is obsolete: true
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.
Attachment #281128 - Flags: superreview?(roc)
Attachment #281128 - Flags: review?(benjamin)
Attachment #281128 - Flags: superreview?(roc) → superreview+

Updated

11 years ago
Attachment #281128 - Flags: review?(benjamin) → review+
Comment on attachment 281128 [details] [diff] [review]
Patch with includes fixed again.

This will help performance ... no idea how much.
Attachment #281128 - Flags: approval1.9?
> 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
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 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!
Attachment #281128 - Flags: approval1.9? → approval1.9+
Checked in. I'll remove nsAutoBuffer after I'm sure it won't need to be reverted.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Doesn't seem to have made a noticeable performance difference. Oh well.

nsAutoBuffer.h removed.
(Assignee)

Comment 26

11 years ago
(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

11 years ago
I found a leftover of nsAutoBuffer
http://mxr.mozilla.org/seamonkey/source/gfx/src/windows/nsFontMetricsWin.cpp#646
intentional?
Not intentional, but not a problem since that code is obsolete and will never be used again.

Comment 29

11 years ago
(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 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());
> 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.

Updated

11 years ago
Depends on: 396761

Updated

11 years ago
Blocks: 396767
You need to log in before you can comment on or make changes to this bug.