Closed
Bug 394691
Opened 17 years ago
Closed 17 years ago
Make nsTArrayElementTraits not do default initialization
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
People
(Reporter: roc, Assigned: cpearce)
References
Details
Attachments
(1 file, 6 obsolete files)
61.92 KB,
patch
|
benjamin
:
review+
roc
:
superreview+
bzbarsky
:
approval1.9+
|
Details | Diff | Splinter Review |
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•17 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•17 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•17 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•17 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.
Reporter | ||
Comment 4•17 years ago
|
||
> 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.
Reporter | ||
Comment 5•17 years ago
|
||
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•17 years ago
|
||
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 7•17 years ago
|
||
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•17 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•17 years ago
|
||
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)
Reporter | ||
Comment 10•17 years ago
|
||
In some places you're including nsAutoTArray.h, but that file doesn't exist --- should be nsTArray.h.
Assignee | ||
Comment 11•17 years ago
|
||
Previous patch with Mac #include's fixed.
Attachment #280704 -
Attachment is obsolete: true
Assignee | ||
Comment 12•17 years ago
|
||
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•17 years ago
|
||
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)
Reporter | ||
Comment 14•17 years ago
|
||
- nsAutoBuffer<UniChar, 32> buffer;
+ nsTAutoArray<UniChar, 32> buffer;
nsAutoTArray
Assignee | ||
Comment 15•17 years ago
|
||
Attachment #280843 -
Attachment is obsolete: true
Reporter | ||
Comment 16•17 years ago
|
||
This patch has a number of "#include "nsAutoTArray.h" ...
Assignee | ||
Comment 17•17 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•17 years ago
|
||
Sorry for the mixups with the previous patches!
Attachment #280848 -
Attachment is obsolete: true
Reporter | ||
Comment 19•17 years ago
|
||
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)
Reporter | ||
Updated•17 years ago
|
Attachment #281128 -
Flags: superreview?(roc) → superreview+
Updated•17 years ago
|
Attachment #281128 -
Flags: review?(benjamin) → review+
Reporter | ||
Comment 20•17 years ago
|
||
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?
![]() |
||
Comment 21•17 years ago
|
||
> 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•17 years ago
|
||
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•17 years ago
|
||
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+
Reporter | ||
Comment 24•17 years ago
|
||
Checked in. I'll remove nsAutoBuffer after I'm sure it won't need to be reverted.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 25•17 years ago
|
||
Doesn't seem to have made a noticeable performance difference. Oh well.
nsAutoBuffer.h removed.
Assignee | ||
Comment 26•17 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•17 years ago
|
||
I found a leftover of nsAutoBuffer
http://mxr.mozilla.org/seamonkey/source/gfx/src/windows/nsFontMetricsWin.cpp#646
intentional?
Reporter | ||
Comment 28•17 years ago
|
||
Not intentional, but not a problem since that code is obsolete and will never be used again.
Comment 29•17 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 30•17 years ago
|
||
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());
Reporter | ||
Comment 31•17 years ago
|
||
> 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.
Depends on: 445711
You need to log in
before you can comment on or make changes to this bug.
Description
•