Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Implement nsAttrAndChildArray::SizeOf()

RESOLVED FIXED in mozilla8

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 542439 [details] [diff] [review]
Patch v1
Attachment #542439 - Flags: review?(jst)
(Assignee)

Updated

6 years ago
Blocks: 667887

Comment 1

6 years ago
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?
(Assignee)

Comment 2

6 years ago
Created attachment 542501 [details] [diff] [review]
Patch v2

Is that better?
Attachment #542439 - Attachment is obsolete: true
Attachment #542501 - Flags: review?(bzbarsky)
Attachment #542439 - Flags: review?(jst)
(Assignee)

Comment 3

6 years ago
Created attachment 542503 [details] [diff] [review]
Patch v3

Updated patch based on our IRC conversation.
Attachment #542501 - Attachment is obsolete: true
Attachment #542503 - Flags: review?(bzbarsky)
Attachment #542501 - Flags: review?(bzbarsky)

Comment 4

6 years ago
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?
(Assignee)

Comment 6

6 years ago
(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.
(Assignee)

Updated

6 years ago
No longer blocks: 667887
(Assignee)

Comment 7

6 years ago
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); }
(Assignee)

Comment 8

6 years ago
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.
(Assignee)

Comment 9

6 years ago
Created attachment 543946 [details] [diff] [review]
Patch v4

Use helpers introduced in bug 669308.
Attachment #542503 - Attachment is obsolete: true
Attachment #543946 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Blocks: 669904

Comment 10

6 years ago
Comment on attachment 543946 [details] [diff] [review]
Patch v4

r=me
Attachment #543946 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

6 years ago
Flags: in-testsuite-
Whiteboard: [needs review] → [inbound]
http://hg.mozilla.org/mozilla-central/rev/b5daeed15a48
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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.