Closed Bug 502176 Opened 10 years ago Closed 9 years ago

[HTML5] Make jArray into a constructorless struct

Categories

(Core :: HTML: Parser, defect, P3)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: hsivonen, Assigned: hsivonen)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Troubles with constructors on statics could be avoided if jArray were a constructorless struct. Need to make it so.
Priority: -- → P3
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.
Blocks: 569629
I'm working on this.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attached patch Make jArray a plain struct (obsolete) — Splinter Review
This patch won't compile alone. It will need a fix for bug 555940, too. (Coming up but not today.)
Attachment #480926 - Flags: review?(tglek)
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.
Attachment #480926 - Attachment is obsolete: true
Attachment #481185 - Flags: review?(tglek)
Attachment #480926 - Flags: review?(tglek)
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.
Attachment #481185 - Flags: review?(tglek) → review+
(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.
Depends on: 555940
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.
Attachment #481185 - Flags: approval2.0?
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.
Attachment #486894 - Flags: review?(tglek)
Attachment #486894 - Flags: review?(tglek) → review+
Attachment #481185 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/e92d70898e22
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/mozilla-central/rev/e92d70898e22
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.