Closed Bug 667883 Opened 8 years ago Closed 8 years ago

Implement nsAttrAndChildArray::SizeOf()

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: mounir, Assigned: mounir)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Patch v1 (obsolete) — Splinter Review
No description provided.
Attachment #542439 - Flags: review?(jst)
Blocks: 667887
The handling of mapped attrs here looks wrong because they're shared.

Also, the attr slots are in mBuffer, so aren't you double-counting them?
Attached patch Patch v2 (obsolete) — Splinter Review
Is that better?
Attachment #542439 - Attachment is obsolete: true
Attachment #542501 - Flags: review?(bzbarsky)
Attachment #542439 - Flags: review?(jst)
Attached patch Patch v3 (obsolete) — Splinter Review
Updated patch based on our IRC conversation.
Attachment #542501 - Attachment is obsolete: true
Attachment #542503 - Flags: review?(bzbarsky)
Attachment #542501 - Flags: review?(bzbarsky)
Comment on attachment 542503 [details] [diff] [review]
Patch v3

>+    size += sizeof(*mImpl);

Take that out; that's what NS_IMPL_EXTRA_SIZE handles for you.

r=me with that.
Attachment #542503 - Flags: review?(bzbarsky) → review+
Comment on attachment 542503 [details] [diff] [review]
Patch v3

+nsGenericElement::SizeOf() const
+{
+  PRInt64 size = Element::SizeOf();
+
+  size -= sizeof(mAttrsAndChildren);
+  size += mAttrsAndChildren.SizeOf();
+
+  return size;

What here takes into account the size of the other members of nsGenericElement? Like the refcount, etc? Or is that dealt with elsewhere?
(In reply to comment #5)
> Comment on attachment 542503 [details] [diff] [review] [review]
> Patch v3
> 
> +nsGenericElement::SizeOf() const
> +{
> +  PRInt64 size = Element::SizeOf();
> +
> +  size -= sizeof(mAttrsAndChildren);
> +  size += mAttrsAndChildren.SizeOf();
> +
> +  return size;
> 
> What here takes into account the size of the other members of
> nsGenericElement? Like the refcount, etc? Or is that dealt with elsewhere?

I will deal with that later.
No longer blocks: 667887
Comment on attachment 542503 [details] [diff] [review]
Patch v3

In nsIContent.h:
>-  PRInt64 SizeOf() const {
>+  virtual PRInt64 SizeOf() const {
>     return sizeof(*this);
>   }

In nsGenericElement.cpp:
>+PRInt64
>+nsGenericElement::SizeOf() const
>+{
>+  PRInt64 size = Element::SizeOf();

I realize this is wrong... :(
nsIContent::SizeOf() is going to return sizeof(nsIContent) even if |this| is nsGenericElement...

This is really annoying because what we are doing is already hardly maintainable but if we have to explicitly add SizeOf() on all the inheritance chain, this is going to be even worse...
Actually, here, the correct call I think should be:
PRInt64 size = Element::SizeOf() + sizeof(*this) - sizeof(Element);
In addition, we will have to add:
Element::SizeOf() { return sizeof(*this); }
I wrote a helper method that could help us make this issue less annoying:

template <class TypeCurrent, class TypeParent>
PRInt64 GetInheritedSize(TypeCurrent* obj) {
  return obj->TypeParent::SizeOf() - sizeof(TypeParent) + sizeof(TypeCurrent);
}

For exemple, nsGenericElement::SizeOf() could be rewritten like this:

>+PRInt64
>+nsGenericElement::SizeOf() const
>+{
>+  PRInt64 size = nsContentUtils::GetInheritedSize(nsGenericElement, Element);
[...]

There are two issue with this:
1. We require devs to call GetInheritedSize
2. We need to have ::SizeOf() implemented in all the heritage chain

I think we can try to solve these two issues by creating a macro that would create a basic SizeOf() method for all supported elements.

A more complex solution would be to make SizeOf pure virtual for non-leaf classes but that might have bigger consequences than we would like.
Attached patch Patch v4Splinter Review
Use helpers introduced in bug 669308.
Attachment #542503 - Attachment is obsolete: true
Attachment #543946 - Flags: review?(bzbarsky)
Blocks: 669904
Comment on attachment 543946 [details] [diff] [review]
Patch v4

r=me
Attachment #543946 - Flags: review?(bzbarsky) → review+
Flags: in-testsuite-
Whiteboard: [needs review] → [inbound]
http://hg.mozilla.org/mozilla-central/rev/b5daeed15a48
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.