Last Comment Bug 667883 - Implement nsAttrAndChildArray::SizeOf()
: Implement nsAttrAndChildArray::SizeOf()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Mounir Lamouri (:mounir)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 663271 669904
  Show dependency treegraph
 
Reported: 2011-06-28 06:17 PDT by Mounir Lamouri (:mounir)
Modified: 2011-07-12 11:35 PDT (History)
2 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (3.50 KB, patch)
2011-06-28 06:17 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2 (3.81 KB, patch)
2011-06-28 09:24 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v3 (3.71 KB, patch)
2011-06-28 09:42 PDT, Mounir Lamouri (:mounir)
bzbarsky: review+
Details | Diff | Splinter Review
Patch v4 (4.68 KB, patch)
2011-07-05 08:05 PDT, Mounir Lamouri (:mounir)
bzbarsky: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-06-28 06:17:16 PDT
Created attachment 542439 [details] [diff] [review]
Patch v1
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-06-28 07:58:34 PDT
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?
Comment 2 Mounir Lamouri (:mounir) 2011-06-28 09:24:39 PDT
Created attachment 542501 [details] [diff] [review]
Patch v2

Is that better?
Comment 3 Mounir Lamouri (:mounir) 2011-06-28 09:42:49 PDT
Created attachment 542503 [details] [diff] [review]
Patch v3

Updated patch based on our IRC conversation.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-06-28 09:50:24 PDT
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.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2011-06-28 12:19:07 PDT
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?
Comment 6 Mounir Lamouri (:mounir) 2011-06-28 12:45:27 PDT
(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.
Comment 7 Mounir Lamouri (:mounir) 2011-06-28 16:18:09 PDT
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); }
Comment 8 Mounir Lamouri (:mounir) 2011-06-30 10:35:42 PDT
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.
Comment 9 Mounir Lamouri (:mounir) 2011-07-05 08:05:35 PDT
Created attachment 543946 [details] [diff] [review]
Patch v4

Use helpers introduced in bug 669308.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2011-07-11 07:47:03 PDT
Comment on attachment 543946 [details] [diff] [review]
Patch v4

r=me

Note You need to log in before you can comment on or make changes to this bug.