Troubles with constructors on statics could be avoided if jArray were a constructorless struct. Need to make it so.

Implementation notes: jArray foo(T*, L) is only used for "static" jArrays. Once constructors have been removed they can be turned into jArray foo = {T*, L}; jArray(L) is only used as a parameter to jArray foo(jArray(L)) or jArray foo; foo = jArray(L). The pair can be replaced with a foo.allocate(L) method. jArray() zeros the members however the array is never used like this so this constructor is doing unnecessary work anyway. J_ARRAY_STATIC needs to be updated to name the variable being defined.

A big part of this problem would go away if the named character names had a dedicated type. Filed as bug 555940.

I'm working on this.

This patch won't compile alone. It will need a fix for bug 555940, too. (Coming up but not today.)

This will be easier to review once I can compile, ping me once 555940 is done please.

Chances are the patch won't apply to the trunk. You can get my entire queue from http://hg.mozilla.org/users/hsivonen_iki.fi/patches/file/80f46a1077ac The patch for bug 555940 applies on top of this one and is needed to compile.

Comment on attachment 481185 [details] [diff] [review] Replace static use of jArray with a plain old data staticJArray, introduce an autoJArray for nicer memory management I see that a lot of these are fairly small allocations. If this is hot code can reorder the struct to be struct jArray { L length; T arr[1]; } then calloc/malloc it at once. This is probably a silly idea. >+nsHtml5AttributeName* nsHtml5AttributeName::ATTR_GLYPH_ORIENTATION_HORIZONTAL = nsnull; I'm not sure if this is portable, but at least on linux uninitialized globals live in .bss, ie are zeroed. So there is no need to explicitly null them. >+staticJArray<PRInt32,PRInt32> nsHtml5ElementName::ELEMENT_HASHES = { ELEMENT_HASHES_DATA, NS_ARRAY_LENGTH(ELEMENT_HASHES_DATA) }; >- jArray<PRUnichar,PRInt32> arr = jArray<PRUnichar,PRInt32>(len); >+ jArray<PRUnichar,PRInt32> arr = jArray<PRUnichar,PRInt32>::newJArray(len); Have you considered using typedefs? I see that there are only a handful of template parameter combinations. giving r+ since the overall structure seems to achieve POD-construction to avoid static initializers. Not qualified on the rest.

(In reply to comment #7) > I see that a lot of these are fairly small allocations. If this is hot code can > reorder the struct to be > struct jArray { > L length; > T arr[1]; > } > then calloc/malloc it at once. This is probably a silly idea. I don't understand the suggestion. The struct doesn't get malloced. It's stack-allocated (and in a special case used as a field of a larger object). Also, it really needs to hold a pointer to a heap-allocated array of T, so T arr[1] wouldn't work. I'm not competent to comment on the merit of reordering the members. What's the expected benefit? > >+nsHtml5AttributeName* nsHtml5AttributeName::ATTR_GLYPH_ORIENTATION_HORIZONTAL = nsnull; > > I'm not sure if this is portable, but at least on linux uninitialized globals > live in .bss, ie are zeroed. So there is no need to explicitly null them. I thought the linker would fail if these static members didn't have definitions. > >+staticJArray<PRInt32,PRInt32> nsHtml5ElementName::ELEMENT_HASHES = { ELEMENT_HASHES_DATA, NS_ARRAY_LENGTH(ELEMENT_HASHES_DATA) }; > > >- jArray<PRUnichar,PRInt32> arr = jArray<PRUnichar,PRInt32>(len); > >+ jArray<PRUnichar,PRInt32> arr = jArray<PRUnichar,PRInt32>::newJArray(len); > Have you considered using typedefs? I see that there are only a handful of > template parameter combinations. > > > giving r+ since the overall structure seems to achieve POD-construction to > avoid static initializers. Not qualified on the rest. Thanks.

> >- jArray<PRUnichar,PRInt32> arr = jArray<PRUnichar,PRInt32>(len); > >+ jArray<PRUnichar,PRInt32> arr = jArray<PRUnichar,PRInt32>::newJArray(len); > Have you considered using typedefs? I see that there are only a handful of > template parameter combinations. Oops. I meant to comment on this bit, too. No, I haven't considered using typedefs. I have considered hard-coding the length type, though. The reason I parametrized the length type originally was that I was trying to make jArray.h usable in non-NSPR contexts without doing a find&replace on the code. Removing the length type parameter might merit a follow-up bug. I think hiding the array type behind a typedef doesn't look like a particular win to me. After all, we don't hide the array type when using nsTArray.

Comment on attachment 481185 [details] [diff] [review] Replace static use of jArray with a plain old data staticJArray, introduce an autoJArray for nicer memory management Requesting approval, because this patch together with the patch for bug 555940 would advance the goal of bug 569629, i.e. getting rid of static initializers on the startup path.

Looks like the patch here introduces a bogus assertion: This code nsAHtml5TreeBuilderState* nsHtml5TreeBuilder::newSnapshot() { jArray<nsHtml5StackNode*,PRInt32> listCopy = jArray<nsHtml5StackNode*,PRInt32>::newJArray(listPtr + 1); can trigger the assertion here static jArray<T,L> newJArray(L const len) { NS_ASSERTION(len > 0, "Bad length."); jArray<T,L> newArray = { new T[len], len }; but the code works right.